[v2] CodingGuidelines: also mention MAYBE_UNUSED

Commit Message

Junio C Hamano Aug. 29, 2024, 6:18 p.m. UTC

A function that uses a parameter in one build may lose all uses of
the parameter in another build, depending on the configuration.  A
workaround for such a case, MAYBE_UNUSED, should also be mentioned
when we recommend the use of UNUSED to our developers.

Keep the addition to the guideline short and document the criteria
to choose between UNUSED and MAYBE_UNUSED near their definition.

Signed-off-by: Junio C Hamano <[e-mail]>
---
 Documentation/CodingGuidelines |  5 +++--
 git-compat-util.h              | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)


Interdiff against v1:
  diff --git a/git-compat-util.h b/git-compat-util.h
  index 23307ce780..e4a306dd56 100644
  --- a/git-compat-util.h
  +++ b/git-compat-util.h
  @@ -196,7 +196,9 @@ struct strbuf;
   #define _SGI_SOURCE 1
   
   /*
  - * UNUSED marks a function parameter that is always unused.
  + * UNUSED marks a function parameter that is always unused.  It also
  + * can be used to annotate a function, a variable, or a type that is
  + * always unused.
    *
    * A callback interface may dictate that a function accepts a
    * parameter at that position, but the implementation of the function
  @@ -662,13 +664,14 @@ static inline int git_has_dir_sep(const char *path)
   
   /*
    * MAYBE_UNUSED marks a function parameter that may be unused, but
  - * whose use is not an error.
  + * whose use is not an error.  It also can be used to annotate a
  + * function, a variable, or a type that may be unused.
    *
  - * 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.
  + * Depending on a configuration, all uses of such a thing may become
  + * #ifdef'ed away.  Marking it with UNUSED would give a warning in a
  + * compilation where it is indeed used, and not marking it at all
  + * would give a warning in a compilation where it is unused.  In such
  + * a case, MAYBE_UNUSED is the appropriate annotation to use.
    */
   #define MAYBE_UNUSED __attribute__((__unused__))

Comments

Jeff King Aug. 29, 2024, 7:40 p.m. UTC | #1

On Thu, Aug 29, 2024 at 11:18:06AM -0700, Junio C Hamano wrote:

> +/*
> + * MAYBE_UNUSED marks a function parameter that may be unused, but
> + * whose use is not an error.  It also can be used to annotate a
> + * function, a variable, or a type that may be unused.
> + *
> + * Depending on a configuration, all uses of such a thing may become
> + * #ifdef'ed away.  Marking it with UNUSED would give a warning in a
> + * compilation where it is indeed used, and not marking it at all
> + * would give a warning in a compilation where it is unused.  In such
> + * a case, MAYBE_UNUSED is the appropriate annotation to use.
> + */
>  #define MAYBE_UNUSED __attribute__((__unused__))

Thanks, I think this is good. There's more nuanced discussion about when
the "MAYBE" variant could be used for non-parameters, but I don't know
that it's worth trying to enumerate every place we've found it useful.

-Peff

Patch

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index d0fc7cfe60..3263245b03 100644
--- a/Documentation/CodingGuidelines
+++ b/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 a/git-compat-util.h b/git-compat-util.h
index 71b4d23f03..e4a306dd56 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -195,6 +195,19 @@  struct strbuf;
 #define _NETBSD_SOURCE 1
 #define _SGI_SOURCE 1
 
+/*
+ * UNUSED marks a function parameter that is always unused.  It also
+ * can be used to annotate a function, a variable, or a type 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 +662,17 @@  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.  It also can be used to annotate a
+ * function, a variable, or a type that may be unused.
+ *
+ * Depending on a configuration, all uses of such a thing may become
+ * #ifdef'ed away.  Marking it with UNUSED would give a warning in a
+ * compilation where it is indeed used, and not marking it at all
+ * would give a warning in a compilation where it is unused.  In such
+ * a case, MAYBE_UNUSED is the appropriate annotation to use.
+ */
 #define MAYBE_UNUSED __attribute__((__unused__))
 
 #include "compat/bswap.h"