| Message ID | xmqqttf3uquc.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] CodingGuidelines: also mention MAYBE_UNUSED | expand |
On Thu, Aug 29, 2024 at 11:27:39AM -0700, Junio C Hamano wrote: > Just like we only define UNUSED macro when __GNUC__ is defined, > and fall back to an empty definition otherwise, we should do the > same for MAYBE_UNUSED. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > * Before I forget that we have discussed this, just as a > documentation (read: this is not a patch to be applied). > > I think this only matters when a compiler satisfies all three > traits: > > - does not define __GNUC__ > - does have its own __attribute__() macro > - barfs on __attribute__((__unused__)) > > Otherwise we will define __attribute__(x) away to empty to cause > no harm. > > Since we have survived without complaints without such a guard > for quite some time, it may be a sign that no compiler that knows > __attribute__() that people ever tried to compile Git with barfs > with __attribute__((__unused__)). I dunno. Yeah, I was surprised that this didn't have a guard and was not currently barfing on other compilers. And the answer is that we already turn __attribute__ into a noop on non-GNUC platforms. Which made me wonder if UNUSED really needs its guards. It does, because it is defined early in the file, before the __attribute__ handling. I don't think we want to move it down, since it needs to be available for use by inline'd compat wrappers. But arguably we should move the attribute macro earlier in the file? I don't know that it is really worth spending too much time futzing with, though. > diff --git a/git-compat-util.h b/git-compat-util.h > index e4a306dd56..74ed581b5d 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -673,7 +673,11 @@ static inline int git_has_dir_sep(const char *path) > * would give a warning in a compilation where it is unused. In such > * a case, MAYBE_UNUSED is the appropriate annotation to use. > */ > +#ifdef __GNUC__ > #define MAYBE_UNUSED __attribute__((__unused__)) > +#else > +#define MAYBE_UNUSED > +#endif So yeah, I'm not necessarily opposed to this, but I don't think it's really doing anything in practice. -Peff
Jeff King <peff@peff.net> writes: > On Thu, Aug 29, 2024 at 11:27:39AM -0700, Junio C Hamano wrote: > >> Just like we only define UNUSED macro when __GNUC__ is defined, >> and fall back to an empty definition otherwise, we should do the >> same for MAYBE_UNUSED. >> >> Signed-off-by: Junio C Hamano <gitster@pobox.com> >> --- >> * Before I forget that we have discussed this, just as a >> documentation (read: this is not a patch to be applied). >> >> I think this only matters when a compiler satisfies all three >> traits: >> >> - does not define __GNUC__ >> - does have its own __attribute__() macro >> - barfs on __attribute__((__unused__)) >> >> Otherwise we will define __attribute__(x) away to empty to cause >> no harm. >> >> Since we have survived without complaints without such a guard >> for quite some time, it may be a sign that no compiler that knows >> __attribute__() that people ever tried to compile Git with barfs >> with __attribute__((__unused__)). I dunno. > > Yeah, I was surprised that this didn't have a guard and was not > currently barfing on other compilers. And the answer is that we already > turn __attribute__ into a noop on non-GNUC platforms. Plus these non-GNUC platforms either (1) do not have their own __attribute__, which lets us turn __attribute__() into noop, or (2) have their own __attribute__, but they happen to support __attribute__((__unused__)). If somebody has __attribute__() and does not support (__unused__) in it, use of MAYBE_UNUSED would be broken (maybe their __attribute__() supports other things but not unused). > Which made me wonder if UNUSED really needs its guards. It does, because > it is defined early in the file, before the __attribute__ handling. I > don't think we want to move it down, since it needs to be available for > use by inline'd compat wrappers. But arguably we should move the > attribute macro earlier in the file? And moving __attribute__ definition earlier in the file would not help such a platform with broken __attribute__((__unused__)) > I don't know that it is really worth spending too much time futzing > with, though. I am inclined to think it is not. So let's scrap the patch. The list archive will hopefully remember when it becomes necessary ;-)
diff --git a/git-compat-util.h b/git-compat-util.h index e4a306dd56..74ed581b5d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -673,7 +673,11 @@ static inline int git_has_dir_sep(const char *path) * would give a warning in a compilation where it is unused. In such * a case, MAYBE_UNUSED is the appropriate annotation to use. */ +#ifdef __GNUC__ #define MAYBE_UNUSED __attribute__((__unused__)) +#else +#define MAYBE_UNUSED +#endif #include "compat/bswap.h"
Just like we only define UNUSED macro when __GNUC__ is defined, and fall back to an empty definition otherwise, we should do the same for MAYBE_UNUSED. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * Before I forget that we have discussed this, just as a documentation (read: this is not a patch to be applied). I think this only matters when a compiler satisfies all three traits: - does not define __GNUC__ - does have its own __attribute__() macro - barfs on __attribute__((__unused__)) Otherwise we will define __attribute__(x) away to empty to cause no harm. Since we have survived without complaints without such a guard for quite some time, it may be a sign that no compiler that knows __attribute__() that people ever tried to compile Git with barfs with __attribute__((__unused__)). I dunno. git-compat-util.h | 4 ++++ 1 file changed, 4 insertions(+)