| Message ID | xmqqseunxtks.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add ref content check for files backend | expand |
On Thu, Aug 29, 2024 at 08:00:19AM -0700, Junio C Hamano wrote: > > Yes, it would be fine to use MAYBE_UNUSED in a case like this. > > It turns out that I was, without realizing it myself, making an > oblique reference to your patch 7/6 ;-) > > Perhaps something along this line? Yeah, this looks good. A few small comments below (but I'm not sure anything needs to be changed). > diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines > index d0fc7cfe60..3263245b03 100644 > --- c/Documentation/CodingGuidelines > +++ w/Documentation/CodingGuidelines > @@ -262,8 +262,9 @@ For C programs: > like "error: unused parameter 'foo' [-Werror=unused-parameter]", > which indicates that a function ignores its argument. If the unused > parameter can't be removed (e.g., because the function is used as a > - callback and has to match a certain interface), you can annotate the > - individual parameters with the UNUSED keyword, like "int foo UNUSED". > + callback and has to match a certain interface), you can annotate > + the individual parameters with the UNUSED (or MAYBE_UNUSED) > + keyword, like "int foo UNUSED". Here I was going to suggest explaining why you'd use one or the other (because I'm afraid of people using MAYBE_UNUSED when UNUSED would be more appropriate). But I think the extra comments you added later are even better, as it lets us explain without cluttering up the CodingGuidelines document. > +/* > + * UNUSED marks a function parameter that is always unused. > + * > + * A callback interface may dictate that a function accepts a > + * parameter at that position, but the implementation of the function > + * may not need to use the parameter. In such a case, mark the parameter > + * with UNUSED. > + * > + * When a parameter may be used or unused, depending on conditional > + * compilation, consider using MAYBE_UNUSED instead. > + */ Looks good. > +/* > + * MAYBE_UNUSED marks a function parameter that may be unused, but > + * whose use is not an error. > + * > + * Depending on a configuration, all uses of a function parameter may > + * become #ifdef'ed away. Marking such a parameter with UNUSED would > + * give a warning in a compilation where the parameter is indeed used, > + * and not marking such a parameter would give a warning in a > + * compilation where the parameter is unused. > + */ > #define MAYBE_UNUSED __attribute__((__unused__)) This is all good as pertains to function parameters. But the original reason we added MAYBE_UNUSED was actually for static functions that were auto-generated by the commit-slab macros. Saying "...marks a function parameter" implies to me that it's the only use. I don't know if we want to be more expansive here or not. Adding auto-generated macro functions should be quite a rarity, I'd think. -Peff
Jeff King <peff@peff.net> writes: >> +/* >> + * MAYBE_UNUSED marks a function parameter that may be unused, but >> + * whose use is not an error. >> + * >> + * Depending on a configuration, all uses of a function parameter may >> + * become #ifdef'ed away. Marking such a parameter with UNUSED would >> + * give a warning in a compilation where the parameter is indeed used, >> + * and not marking such a parameter would give a warning in a >> + * compilation where the parameter is unused. >> + */ >> #define MAYBE_UNUSED __attribute__((__unused__)) > > This is all good as pertains to function parameters. But the original > reason we added MAYBE_UNUSED was actually for static functions that were > auto-generated by the commit-slab macros. Saying "...marks a function > parameter" implies to me that it's the only use. I don't know if we want > to be more expansive here or not. Adding auto-generated macro functions > should be quite a rarity, I'd think. True. You can annotate types, variables, and functions with the attributes as well. How about saying something like this MAYBE_UNUSED marks a function parameter that may be unused but whose use is not an error. It also can be applied to functions, types and variables. and then keep the explanation of why you may want to use the maybe- variant as-is, using a function parameter as an example? Or I could rewrite "parameter" and "function parameter" in it with "thing" (with double quotes around), like: Depending on a configuration, all uses of a "thing" may become #ifdef'ed away.... Unlike the use of deprecated attribute, our definition of MAYBE_UNUSED is not guarded with anything. Shouldn't we at least do #if defined(__GNUC__) #define MAYBE_UNUSED __attribute__((__unused__)) #else #define MAYBE_UNUSED /* noop */ #endif or something, by the way? Thanks.
diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines index d0fc7cfe60..3263245b03 100644 --- c/Documentation/CodingGuidelines +++ w/Documentation/CodingGuidelines @@ -262,8 +262,9 @@ For C programs: like "error: unused parameter 'foo' [-Werror=unused-parameter]", which indicates that a function ignores its argument. If the unused parameter can't be removed (e.g., because the function is used as a - callback and has to match a certain interface), you can annotate the - individual parameters with the UNUSED keyword, like "int foo UNUSED". + callback and has to match a certain interface), you can annotate + the individual parameters with the UNUSED (or MAYBE_UNUSED) + keyword, like "int foo UNUSED". - We try to support a wide range of C compilers to compile Git with, including old ones. As of Git v2.35.0 Git requires C99 (we check diff --git c/git-compat-util.h w/git-compat-util.h index 71b4d23f03..23307ce780 100644 --- c/git-compat-util.h +++ w/git-compat-util.h @@ -195,6 +195,17 @@ struct strbuf; #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +/* + * UNUSED marks a function parameter that is always unused. + * + * A callback interface may dictate that a function accepts a + * parameter at that position, but the implementation of the function + * may not need to use the parameter. In such a case, mark the parameter + * with UNUSED. + * + * When a parameter may be used or unused, depending on conditional + * compilation, consider using MAYBE_UNUSED instead. + */ #if GIT_GNUC_PREREQ(4, 5) #define UNUSED __attribute__((unused)) \ __attribute__((deprecated ("parameter declared as UNUSED"))) @@ -649,6 +660,16 @@ static inline int git_has_dir_sep(const char *path) #define RESULT_MUST_BE_USED #endif +/* + * MAYBE_UNUSED marks a function parameter that may be unused, but + * whose use is not an error. + * + * Depending on a configuration, all uses of a function parameter may + * become #ifdef'ed away. Marking such a parameter with UNUSED would + * give a warning in a compilation where the parameter is indeed used, + * and not marking such a parameter would give a warning in a + * compilation where the parameter is unused. + */ #define MAYBE_UNUSED __attribute__((__unused__)) #include "compat/bswap.h"