{"thread":{"id":"47649","subject":"[Nit] Lots of enumerated type warnings","startedAt":"2018-01-19T23:18:20Z","lastAt":"2018-01-23T00:06:11Z","messageCount":5,"participants":["Randall S. Becker","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"336956","messageId":"001601d3917b$c1ade3c0$4509ab40$@nexbridge.com","threadId":"47649","inReplyTo":null,"subject":"[Nit] Lots of enumerated type warnings","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-19T23:17:58Z","receivedAt":"2018-01-19T23:18:20Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"So here a bit of a nit or nano-quibble that I have. Call it my \"warnings\nOCD\" if you want. I'm seeing an increase in the enumerated type warnings\ncoming from my use of the c99 compiler for compiling git over time (loads\nmore for 2.16.0 compared to 2.3.7 when I took it on). What is the general\nfeeling on these? I would be willing do static casts rather than see the\nwarnings, mostly because I advocate in public that warnings are actually\nfuture potential errors, so clean compiles are better. I don't see this\nconflicting with anything in gcc. Is there a desire/need to clean up this\nstuff? I can take a stab at gradually cleaning this up when $DAYJOB and\n#FAMILY don't conflict.\n\nAlthough, given the choice, I'd rather look into that whole --via concept\nfrom a different thread ;-)\n\nCheers,\nRandall\n\n-- Brief whoami:\n NonStop developer since approximately 211288444200000000\n UNIX developer since approximately 421664400\n-- In my real life, I talk too much.\n\n\n\n"},{"id":"337110","messageId":"xmqqo9lljz3l.fsf@gitster.mtv.corp.google.com","threadId":"47649","inReplyTo":"001601d3917b$c1ade3c0$4509ab40$@nexbridge.com","subject":"Re: [Nit] Lots of enumerated type warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-22T22:41:18Z","receivedAt":"2018-01-22T22:41:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n> I'm seeing an increase in the enumerated type warnings\n> coming from my use of the c99 compiler for compiling git over time (loads\n> more for 2.16.0 compared to 2.3.7 when I took it on).\n\nWhat exactly do these \"warnings\" complain about?  Without knowing\nthat, the remainder of your question cannot be answered.\n\nDoes it complain against enum FOO {A,B,C,} saying that the comma\nafter C is not kosher in older C standard, for example?\n\n"},{"id":"337113","messageId":"001501d393d3$b82150d0$2863f270$@nexbridge.com","threadId":"47649","inReplyTo":"xmqqo9lljz3l.fsf@gitster.mtv.corp.google.com","subject":"RE: [Nit] Lots of enumerated type warnings","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-22T22:52:40Z","receivedAt":"2018-01-22T22:52:55Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 22, 2018 5:41 PM, Junio C Hamano wrote:\n> \"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n> \n> > I'm seeing an increase in the enumerated type warnings coming from my\n> > use of the c99 compiler for compiling git over time (loads more for\n> > 2.16.0 compared to 2.3.7 when I took it on).\n> \n> What exactly do these \"warnings\" complain about?  Without knowing that,\n> the remainder of your question cannot be answered.\n> \n> Does it complain against enum FOO {A,B,C,} saying that the comma after C\nis\n> not kosher in older C standard, for example?\n\nHere are a few examples, there are more:\n\nauto_crlf = git_config_bool(var, value);\n  \t\t          ^\n\"/home/jenkins/.jenkins/workspace/Build_Git/config.c\", line 1147:\nwarning(272): \n          enumerated type mixed with another type\n\ntype = sha1_object_info(s->oid.hash, &s->size);\n  \t\t\t     ^\n\"/home/jenkins/.jenkins/workspace/Build_Git/diff.c\", line 3618:\nwarning(272): \n          enumerated type mixed with another type\n\noptions->color_moved = diff_color_moved_default;\n  \t                     ^\n\"/home/jenkins/.jenkins/workspace/Build_Git/diff.c\", line 4108:\nwarning(272): \n          enumerated type mixed with another type\n\noptions->color_moved = 0;\n  \t\t                     ^\n\"/home/jenkins/.jenkins/workspace/Build_Git/diff.c\", line 4218:\nwarning(272): \n          enumerated type mixed with another type\n\n"},{"id":"337123","messageId":"xmqqy3kpihm3.fsf@gitster.mtv.corp.google.com","threadId":"47649","inReplyTo":"001501d393d3$b82150d0$2863f270$@nexbridge.com","subject":"Re: [Nit] Lots of enumerated type warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-01-22T23:44:20Z","receivedAt":"2018-01-22T23:44:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n\n> Here are a few examples, there are more:\n>\n> auto_crlf = git_config_bool(var, value);\n>   \t\t          ^\n\nThe carets in your message do not align to what I think they are\ntrying to point at, but I think the above is pointing at the '=' and\nwants to say \"auto_crlf variable is enum, it gets assigned an\ninteger returned from git_config_bool(), and I do not like that\nassignment\".\n\nFor this one I tend to agree with the compiler, meaning that it is\nugly to define \"enum auto_crlf\" in such a way that the first two\nvalues happen to match what a logically different \"enum\" (which is\n\"boolean\") assigns the two values to.  And this judgment does not\nchange whether git_config_bool() actually returns an enum or an int\n(the code in reality returns the latter).\n\nI do not think people would terribly mind a patch to turn the above\ninto:\n\n  auto_crlf = git_config_bool(var, value) ? AUTO_CRLF_FALSE : AUTO_CRLF_TRUE;\n\n> \"/home/jenkins/.jenkins/workspace/Build_Git/config.c\", line 1147:\n> warning(272): \n>           enumerated type mixed with another type\n>\n> type = sha1_object_info(s->oid.hash, &s->size);\n>   \t\t\t     ^\n\n/* returns enum object_type or negative */\nint sha1_object_info(const unsigned char *sha1, unsigned long *sizep)\n\nThe function has been like this forever, I suspect, and I would say\n\"this gives negative when error, or enum we know is non-negative\" is\nquite a reasonable thing to do, but the enum has OBJ_BAD defined to\nbe negative, so probably it is more kosher if sha1_object_info() is\ndeclared to return \"enum object_type\" instead of int.\n\n> \"/home/jenkins/.jenkins/workspace/Build_Git/diff.c\", line 3618:\n> warning(272): \n>           enumerated type mixed with another type\n>\n> options->color_moved = diff_color_moved_default;\n>   \t                     ^\n\nIf color_moved field is declared to be an enum, the _default\nvariable should be, too.  I do not think it is such a controversial\nfix.\n"},{"id":"337128","messageId":"001a01d393dd$f5a65310$e0f2f930$@nexbridge.com","threadId":"47649","inReplyTo":"xmqqy3kpihm3.fsf@gitster.mtv.corp.google.com","subject":"RE: [Nit] Lots of enumerated type warnings","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2018-01-23T00:05:58Z","receivedAt":"2018-01-23T00:06:11Z","isPatch":false,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On January 22, 2018 6:44 PM, Junio C Hamano wrote:\n> \n> \"Randall S. Becker\" <rsbecker@nexbridge.com> writes:\n> \n> > Here are a few examples, there are more:\n> >\n> > auto_crlf = git_config_bool(var, value);\n> >   \t\t          ^\n> \n> The carets in your message do not align to what I think they are trying to\n> point at, but I think the above is pointing at the '=' and wants to say\n> \"auto_crlf variable is enum, it gets assigned an integer returned from\n> git_config_bool(), and I do not like that assignment\".\n> \n> For this one I tend to agree with the compiler, meaning that it is ugly to\n> define \"enum auto_crlf\" in such a way that the first two values happen to\n> match what a logically different \"enum\" (which is\n> \"boolean\") assigns the two values to.  And this judgment does not change\n> whether git_config_bool() actually returns an enum or an int (the code in\n> reality returns the latter).\n> \n> I do not think people would terribly mind a patch to turn the above\n> into:\n> \n>   auto_crlf = git_config_bool(var, value) ? AUTO_CRLF_FALSE :\n> AUTO_CRLF_TRUE;\n> \n> > \"/home/jenkins/.jenkins/workspace/Build_Git/config.c\", line 1147:\n> > warning(272):\n> >           enumerated type mixed with another type\n> >\n> > type = sha1_object_info(s->oid.hash, &s->size);\n> >   \t\t\t     ^\n> \n> /* returns enum object_type or negative */ int sha1_object_info(const\n> unsigned char *sha1, unsigned long *sizep)\n> \n> The function has been like this forever, I suspect, and I would say \"this\ngives\n> negative when error, or enum we know is non-negative\" is quite a\n> reasonable thing to do, but the enum has OBJ_BAD defined to be negative,\n> so probably it is more kosher if sha1_object_info() is declared to return\n> \"enum object_type\" instead of int.\n> \n> > \"/home/jenkins/.jenkins/workspace/Build_Git/diff.c\", line 3618:\n> > warning(272):\n> >           enumerated type mixed with another type\n> >\n> > options->color_moved = diff_color_moved_default;\n> >   \t                     ^\n> \n> If color_moved field is declared to be an enum, the _default variable\nshould\n> be, too.  I do not think it is such a controversial fix.\n\nThe basic idea of the request is whether to slowly take on this type of\nchange. It will likely take a bit of time, but I really don't like warnings,\nso am willing to work on it. There are loads more like this that might need\ndiscussion, so I'll be pretty conservative on this effort.\n\nCheers,\nRandall\n\n"}]}