{"thread":{"id":"60947","subject":"[GSoC] Use unsigned integral type for collection of bits","startedAt":"2024-02-18T11:37:21Z","lastAt":"2024-02-20T00:32:47Z","messageCount":4,"participants":["eugenio gigante","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"488871","messageId":"CAFJh0PTiN18LcKP6LW0d1u2gkodBD2cOJRBzU_subBaFpzM=CA@mail.gmail.com","threadId":"60947","inReplyTo":null,"subject":"[GSoC] Use unsigned integral type for collection of bits","fromName":"eugenio gigante","fromEmail":"giganteeugenio2@gmail.com","sentAt":"2024-02-18T11:36:57Z","receivedAt":"2024-02-18T11:37:21Z","isPatch":false,"sender":{"key":"giganteeugenio2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116371051?v=4"},"body":"Hi all,\nI was looking around the codebase for some field of a structure that\nstores collections of bits with signed int type.\nI used this simple grep command to search for it:\n\n$ grep -r -n \"\\tsigned int\" .\n> ./diffcore.h:63:     signed int is_binary : 2;\n\nThe struct in question is \"diff_filespec\" and Junio commented the\ndeclaration of the field as following:\n\n/* data should be considered \"binary\"; -1 means \"don't know yet\" */\n\nSo, if I understood it correctly, possible values are:\n 1 -> 01\n 2 -> 10\n-1 -> 11\nOn the other, by changing it to unsigned values would be:\n1 -> 01\n2 -> 10\n3 -> 11\n\nI read somewhere that one should always prefer unsigned integral type over\n\nsigned integral type for a couple of reasons [1].\n\nThese involve operations like Modulus, Shifting and Overflow.\n\nI didn't dig too much into how the field is used and if there are cases in which\n\nthe mentioned operations are involved (I would like the community\nopinion about this topic before).\n\n\nMoreover, I don’t know if such a change breaks too much code and if\nit’s worth it.\n\nProbably it's not that tragic since the header 'diffcore.h' is only\nincluded in two other files,\n\nbut maybe I'm missing something. For sure, various If conditions used\nby the function\n\n'diff_filespec_is_binary' inside 'diff.c' would have to be changed.\n\n\nBesides, it's possible that my grep command is not enough and maybe\nmore \"signed int\" can be spotted.\n\nThanks!\nEugenio Gigante.\nP.S. I was insecure about how to send this email since it does not\ninclude a commit.\nI decide not to use git-send-email. Hoping I didn't mess up the format.\n\n[1]\nhttps://embeddedgurus.com/stack-overflow/2009/05/signed-versus-unsigned-integers/\n"},{"id":"488880","messageId":"CAPig+cR5K=pQjK+7ZUyGn1M50RZ0pRD6kOPQgmp7qez_LNXcAg@mail.gmail.com","threadId":"60947","inReplyTo":"CAFJh0PTiN18LcKP6LW0d1u2gkodBD2cOJRBzU_subBaFpzM=CA@mail.gmail.com","subject":"Re: [GSoC] Use unsigned integral type for collection of bits","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-02-18T19:09:40Z","receivedAt":"2024-02-18T19:09:52Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 18, 2024 at 6:37 AM eugenio gigante\n<giganteeugenio2@gmail.com> wrote:\n> I was looking around the codebase for some field of a structure that\n> stores collections of bits with signed int type.\n>\n> > ./diffcore.h:63:     signed int is_binary : 2;\n>\n> The struct in question is \"diff_filespec\" and Junio commented the\n> declaration of the field as following:\n>\n> /* data should be considered \"binary\"; -1 means \"don't know yet\" */\n>\n> I read somewhere that one should always prefer unsigned integral type over\n> signed integral type for a couple of reasons [1].\n> These involve operations like Modulus, Shifting and Overflow.\n\nIn the context of Git, we want to be using `unsigned` for variables\nwhich are \"bags of bits\", where each bit indicates some \"on\" or \"off\"\nproperty. Very frequently, such variables have \"flags\" in their names.\nSo, a possible scenario might be something like this:\n\n    #define OP_FOO 0x01\n    #define OP_BAR 0x02\n    #define OP_ZAZ 0x04\n    ...\n    unsigned int flags = OP_FOO | OP_ZAZ;\n    ...\n    if ((flags & OP_ZAZ))\n        do_some_zaz();\n\n> I didn't dig too much into how the field is used and if there are cases in which\n> the mentioned operations are involved (I would like the community\n> opinion about this topic before).\n>\n> Moreover, I don’t know if such a change breaks too much code and if\n> it’s worth it.\n>\n> but maybe I'm missing something. For sure, various If conditions used\n> by the function\n>\n> 'diff_filespec_is_binary' inside 'diff.c' would have to be changed.\n\nThe code in question is not being used as a \"bag of bits\". Rather,\nit's a tristate binary with values \"not-set\", \"true\", and \"false\".\nWhereas a typical binary could be represented by a single bit, this\none needs the extra bit to handle the \"not-set\" case. Moreover, it is\nidiomatic in the Git codebase for -1 to represent \"not-set\", so I\nthink this code is fine as-is since its meaning is clear to those\nfamiliar with the codebase, thus does not need any changes made to it.\n\n> Besides, it's possible that my grep command is not enough and maybe\n> more \"signed int\" can be spotted.\n\nThere are cases in the codebase in which a signed type is being used\nas a \"bag of bits\" instead of the more desirable unsigned type. If you\nare interested in making such a fix, you might find some candidates\nusing a search such as this:\n\n    git grep -P '(?<!unsigned )int\\s+flags'\n\nFor example, it finds this instance in `builtin/add.c`:\n\n    static int refresh(int verbose, const struct pathspec *pathspec)\n    {\n        int flags = REFRESH_IGNORE_SKIP_WORKTREE |\n            (verbose ? REFRESH_IN_PORCELAIN : REFRESH_QUIET);\n        ...\n        refresh_index(&the_index, flags, pathspec, seen, ...);\n    }\n\nTaking a look at `read-cache-ll.h`, we see:\n\n    int refresh_index(struct index_state *, unsigned int flags, ...);\n\nSo, refresh_index() is correctly expecting an unsigned value for\n`flags` but refresh() in `builtin/add.c` has undesirably declared\n`flags` as signed.\n\n> P.S. I was insecure about how to send this email since it does not\n> include a commit.\n> I decide not to use git-send-email. Hoping I didn't mess up the format.\n\nUsing your preferred email client for general discussion is fine. Most\npeople only use git-send-email for sending patches.\n"},{"id":"488938","messageId":"CAFJh0PQPB3DBfxhLhzYgY212yzXOtRt_TtU=7B=SsBOH_iScVQ@mail.gmail.com","threadId":"60947","inReplyTo":"CAFJh0PRJkVBr-A=UtmEcAh4cPgC3w_vdTPg6kkjgHVQXHTYRmA@mail.gmail.com","subject":"Re: [GSoC] Use unsigned integral type for collection of bits","fromName":"eugenio gigante","fromEmail":"giganteeugenio2@gmail.com","sentAt":"2024-02-19T23:43:16Z","receivedAt":"2024-02-19T23:43:27Z","isPatch":false,"sender":{"key":"giganteeugenio2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116371051?v=4"},"body":"On Sun, Feb 18, 2024 at 20:09 AM eric sunshine\n<sunshine@sunshineco.com> wrote:\n> The code in question is not being used as a \"bag of bits\". Rather,\n> it's a tristate binary with values \"not-set\", \"true\", and \"false\".\n> Whereas a typical binary could be represented by a single bit, this\n> one needs the extra bit to handle the \"not-set\" case. Moreover, it is\n> idiomatic in the Git codebase for -1 to represent \"not-set\", so I\n> think this code is fine as-is since its meaning is clear to those\n> familiar with the codebase, thus does not need any changes made to it.\n\nThank you for the clarification and sorry for the misunderstanding.\n\n> So, refresh_index() is correctly expecting an unsigned value for\n> `flags` but refresh() in `builtin/add.c` has undesirably declared\n> `flags` as signed.\n\nSo, an unsigned type is preferable since we are dealing\nwith 'bags of bits', and probably only bitwise operators operate\non them. Also the mixing is not ideal.\nYes, I'm interested in fixing the one in `builtin/add.c`.\n\n\nIl giorno mar 20 feb 2024 alle ore 00:39 eugenio gigante\n<giganteeugenio2@gmail.com> ha scritto:\n>\n> On Sun, Feb 18, 2024 at 20:09 AM eric sunshine\n> <sunshine@sunshineco.com> wrote:\n> > The code in question is not being used as a \"bag of bits\". Rather,\n> > it's a tristate binary with values \"not-set\", \"true\", and \"false\".\n> > Whereas a typical binary could be represented by a single bit, this\n> > one needs the extra bit to handle the \"not-set\" case. Moreover, it is\n> > idiomatic in the Git codebase for -1 to represent \"not-set\", so I\n> > think this code is fine as-is since its meaning is clear to those\n> > familiar with the codebase, thus does not need any changes made to it.\n>\n> Thank you for the clarification and sorry for the misunderstanding.\n>\n> > So, refresh_index() is correctly expecting an unsigned value for\n> > `flags` but refresh() in `builtin/add.c` has undesirably declared\n> > `flags` as signed.\n>\n> So, an unsigned type is preferable since we are dealing\n> with 'bags of bits', and probably only bitwise operators operate\n> on them. Also the mixing is not ideal.\n> Yes, I'm interested in fixing the one in `builtin/add.c`.\n"},{"id":"488942","messageId":"xmqq1q98ynyr.fsf@gitster.g","threadId":"60947","inReplyTo":"CAPig+cR5K=pQjK+7ZUyGn1M50RZ0pRD6kOPQgmp7qez_LNXcAg@mail.gmail.com","subject":"Re: [GSoC] Use unsigned integral type for collection of bits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-02-20T00:32:44Z","receivedAt":"2024-02-20T00:32:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> 'diff_filespec_is_binary' inside 'diff.c' would have to be changed.\n>\n> The code in question is not being used as a \"bag of bits\". Rather,\n> it's a tristate binary with values \"not-set\", \"true\", and \"false\".\n> Whereas a typical binary could be represented by a single bit, this\n> one needs the extra bit to handle the \"not-set\" case. Moreover, it is\n> idiomatic in the Git codebase for -1 to represent \"not-set\", so I\n> think this code is fine as-is since its meaning is clear to those\n> familiar with the codebase, thus does not need any changes made to it.\n\nCorrect.  In general, bitfield structure members in our codebase\nshould be already fine.  Most of them are \"unsigned : 1\" and there\nis not enough room in a single-bit bitfield to go signed.\n\n> There are cases in the codebase in which a signed type is being used\n> as a \"bag of bits\" instead of the more desirable unsigned type.\n> ...\n> So, refresh_index() is correctly expecting an unsigned value for\n> `flags` but refresh() in `builtin/add.c` has undesirably declared\n> `flags` as signed.\n\nThanks for a good example.  In a signed flag word that is used as a\nbag of bits, the MSB is special, and unless you take advantage of\nthat special casing (which happens almost never), you should use an\nunsigned word instead, to document that you are not doing anything\nfunny with the MSB, like \"the flag word is negative, so the MSB must\nbe on\", \"I want to copy the bit immediately below MSB to MSB\", etc.\n"}]}