Bug 207959
Summary: | Don't warn about the universal zero initializer for a structure with the 'designated_init' attribute. | ||
---|---|---|---|
Product: | Tools | Reporter: | Asher Gordon (AsDaGo) |
Component: | Sparse | Assignee: | Tools-Sparse Virtual Assignee (tools_sparse) |
Status: | RESOLVED CODE_FIX | ||
Severity: | enhancement | CC: | luc.vanoostenryck, torvalds |
Priority: | P1 | ||
Hardware: | All | ||
OS: | Linux | ||
Kernel Version: | Sparse 0.6.1 (Debian: 0.6.1-2+b1) | Subsystem: | |
Regression: | No | Bisected commit-id: | |
Attachments: | A test program illustrating the issue |
Description
Asher Gordon
2020-05-28 16:27:17 UTC
In fact, sparse already support this via the option '-Wno-universal-initializer'. It's really very recent and thus only in the mainline tree, not in a release (and it was introduced for another warning but the result is the same). My very personal point of view is that the correct syntax should be '{ }' because it conveys much better the idea of a default initializer. This single zero in '{ 0 }' is just confusing. (In reply to Luc Van Oostenryck from comment #1) > In fact, sparse already support this via the option > '-Wno-universal-initializer'. Perhaps '-Wno-universal-initializer' should be the default? > My very personal point of view is that the correct syntax should be '{ }' > because it conveys much better the idea of a default initializer. This > single zero in '{ 0 }' is just confusing. I can see your point, but unfortunately, as Ramsay Jones says here[1] and Alexander Monakov here[2], this is not standard C. So '{ }' isn't an option if we want to be portable. Andrew Pinski's suggestion[3] is also an option, but that seems ugly to me. [1] https://marc.info/?l=linux-sparse&m=159069587406366&w=2 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379#c4 [3] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379#c1 I'm writing a library, Mu[4], which has a structure for which the 'designated_init' attribute is appropriate (see the 'MU_OPT' structure here[5]). However, I don't want to force my users not to use '{ 0 }', which is why I think this feature would be useful. [4] https://nongnu.org/libmu/ [5] https://git.savannah.nongnu.org/cgit/libmu.git/tree/src/options.h#n85 Also, a minor note: In the test program I attached, the attribute needs to be specified after the closing brace to work with Sparse. (In reply to Asher Gordon from comment #2) > Perhaps '-Wno-universal-initializer' should be the default? Well, that's really the point. The problem Sparse also gives the warnings corresponding to clang's -Wnonnull and my understanding is that these warnings are desired for the kernel even when coming from using '{ 0 }'. > > My very personal point of view is that the correct syntax should be '{ }' > > because it conveys much better the idea of a default initializer. This > > single zero in '{ 0 }' is just confusing. > > I can see your point, but unfortunately, as Ramsay Jones says here[1] and > Alexander Monakov here[2], this is not standard C. So '{ }' isn't an option > if we want to be portable. Yes, I know, it's a pity. It's why I said 'should be'. > Andrew Pinski's suggestion[3] is also an option, > but that seems ugly to me. Yes, it's far from ideal. > I'm writing a library, Mu[4], which has a structure for which the > 'designated_init' attribute is appropriate (see the 'MU_OPT' structure > here[5]). However, I don't want to force my users not to use '{ 0 }', which > is why I think this feature would be useful. Interesting. Yes, I understand. Git was in the same kind of situation, it's why I added '-Wno-universal-initializer'. Can't you just add this option in your SPARSE_FLAGS or something like that? > Also, a minor note: In the test program I attached, the attribute needs to > be specified after the closing brace to work with Sparse. Yes, it's a known problem. Sparse accept 'type attributes' (those situated just after the keyword 'struct', 'union' or 'enum') but ignore them. I've some unfinished patches for this ... since some time already :( On Thu, May 28, 2020 at 2:24 PM <bugzilla-daemon@bugzilla.kernel.org> wrote: > > Well, that's really the point. > The problem Sparse also gives the warnings corresponding to clang's -Wnonnull > and my understanding is that these warnings are desired for the kernel even > when coming from using '{ 0 }'. In the kernel, the empty initializer is be the usual way to create a zero initializer. So yes, { 0 } may exist, but it generally should be used for initializing something that is known to be an integer. And if it's a pointer, it should warn, because '0' should never have been a valid pointer, traditional C or not. It's not like we use pedantic and portable standard C to begin with. So yeah, the sparse defaults may be a bit kernel-centric. Linus That said, I'm not sure the kernel cares. If sparse makes '{ 0 }' be eqivalent to '{ }' and doesn't warn for it, it's not like it's a huge deal. The problem with using 0 instead of NULL (or vice versa, which is a crime, and which is why NULL should never have been defined to plain 0) comes when it is actually confusing. For something like a silly zero struct initializer it's not like it's the end of the world. I do find the whole "0 could be a pointer, and NULL could be used for an integer or float" to be very distasteful, and the C++ people in particular were in denial about their brokenness for much much too long. So I'd prefer the "0 for NULL" warning, even if this may not be the most important case for it. (In reply to Luc Van Oostenryck from comment #3) > Can't you just add this option in your > SPARSE_FLAGS or something like that? Well, actually I'm not using Sparse for my project. I want to use the 'designated_init' attribute since it is supported by GCC. And I want to use the attribute mainly so that users of my library get the warning, and I can't control what flags the user uses (and GCC doesn't have a -Wno-universal-initializer flag anyway). (In reply to Linus Torvalds from comment #4) > So yeah, the sparse defaults may be a bit kernel-centric. That's fine, but perhaps GCC should add something like -Wno-universal-initializer and use it by default. I'll suggest that in the GCC bug (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95379). (In reply to Linus Torvalds from comment #5) > That said, I'm not sure the kernel cares. If sparse makes '{ 0 }' be > equivalent to '{ }' and doesn't warn for it, it's not like it's a huge deal. > > The problem with using 0 instead of NULL (or vice versa, which is a crime, > and which is why NULL should never have been defined to plain 0) comes when > it is actually confusing. OK. I also detest this 'you can use 0 for pointers' but I think that '{ 0 }' should just be understood as the standard idiom for '{ }' and that the current situation where '{ 0 }' gives warnings while '{ }' doesn't s confusing and annoying. So, I'll change Sparse's default to -Wno-universal-initializer. > So I'd prefer the "0 for NULL" warning, even if this may not be the most > important case for it. Do you think it's worth to add -Wuniversal-initializer for the kernel so that these warnings are still present for '{ 0 }'? -- Luc I'm happy to let the kernel not warn about {0} used for a pointer until I find some egregious horror-case, and then I know that I can add -Wuniversal-initializer to warn about it. |