bug-gnulib@gnu.org mirror (unofficial)
 help / color / mirror / Atom feed
From: Bruno Haible <bruno@clisp.org>
To: bug-gnulib@gnu.org
Cc: Paul Eggert <eggert@cs.ucla.edu>
Subject: Re: Using C2x attributes more effectively in Gnulib
Date: Sun, 08 Aug 2021 03:40:57 +0200	[thread overview]
Message-ID: <37018291.SK6AWJdEcI@omega> (raw)
In-Reply-To: <0ed0a5a4-ed41-7d40-1c31-f422b55e7ab3@cs.ucla.edu>

Hi Paul,

> Because draft C2x requires using [[maybe_unused]] before a parameter 
> instead of after, the recent C2x-related changes to Gnulib caused me to 
> move all uses of _GL_UNUSED_PARAMETER

Agreed. We should follow standards. The new standard picked a different
position for the marker than what we used to have.

> _GL_ATTRIBUTE_MAYBE_UNUSED is too long, so I propose we rename 
> it to something shorter.

In implementation code, it is more convenient to include "attribute.h" and
then use MAYBE_UNUSED. The names with _GL_ prefix are, IMO, only for
places where including "attribute.h" is not desired, namely in public
header files. And in public header files we don't have many function
definitions that ignore some arguments. It's basically only
  lib/se-context.in.h
  lib/se-label.in.h
  lib/se-selinux.in.h
IMO that's not enough justification for introducing a new name for
_GL_ATTRIBUTE_MAYBE_UNUSED.

> Also, draft C2x lets one write the above function without naming the 
> parameters, as follows:
> 
>    SE_SELINUX_INLINE int
>    fsetfilecon (int, char const *)
>      { errno = ENOTSUP; return -1; }
> 
> This is nicer than [[maybe_unused]], because it says the arguments are 
> *definitely* unused instead of merely *maybe* unused, and that allows a 
> bit more checking of the code.

I disagree on this one. For a human reader who wants to understand the code,
the parameter name is more important than its type. Therefore, for
maintainability your example should better read

     SE_SELINUX_INLINE int
     fsetfilecon (fd, context)
       { errno = ENOTSUP; return -1; }

In other words, it is a misfeature in the standard. They have designed
this language feature from the perspective what the compiler needs in
order to generate machine code, not from the perspective of the programmer
and maintainability.

So, we should better not use this misfeature.

> So, how about the following ideas:
> 
> * Rename _GL_ATTRIBUTE_MAYBE_UNUSED to _GL_maybe_unused. Similarly for 
> _GL_deprecated, _GL_fallthrough, and _GL_nodiscard.

Ohhhhhh. This breaks a decades-long convention. We have the convention
in GNU that macro names are upper case, to distinguish them from function
and variable names. This is good convention.

I have personally used mixed-case macro names in GNU clisp, and I can
assert (in hindsight) that it did *not* contribute to maintainability.

> As C2x becomes more 
> popular, it'll be easy to read _GL_deprecated as shorthand for 
> "[[deprecated]] if supported, empty otherwise".

Whether the expansion is mainly lowercase or not, is irrelevant.
Breaking said convention is not good.

> * Remove all uses of _GL_UNUSED after arguments in Gnulib, replacing 
> them with _GL_maybe_unused before arguments. This will support non-GCC 
> C2x compilers better.  Deprecate _GL_UNUSED.

Hmm. This _GL_UNUSED macro is used a lot in .c code, simply because it
it short (and the "maybe" in _GL_ATTRIBUTE_MAYBE_UNUSED is more of a
compiler technicality). I'm OK with moving all _GL_UNUSED from after
the parameter declaration to before the parameter declaration. Then
we can continue to have
  #define _GL_UNUSED _GL_ATTRIBUTE_MAYBE_UNUSED

> * Define a macro _GL_UNUSED_ARG(TYPE, NAME) that expands to 'TYPE' in 
> draft C2x, and to '_GL_maybe_unused TYPE NAME' otherwise. That way, one 
> can write:
> 
>    SE_SELINUX_INLINE int
>    fsetfilecon (_GL_UNUSED_ARG (int, fd),
>                 _GL_UNUSED_ARG (char const *, con))
>      { errno = ENOTSUP; return -1; }

Such macros with type parameters have two problems:

  * Maintainability problem: The reader needs to understand the macro
    first, before they can understand the declaration.
    Whereas when the declaration reads

       SE_SELINUX_INLINE int
       fsetfilecon (_GL_UNUSED int fd, _GL_UNUSED char const *con)

    the reader understands 70% of the syntax and can treat _GL_UNUSED
    as "some token I don't need to know about".

  * It doesn't apply to function types that are not typedefs. How would
    you write

              int foo (_GL_UNUSED int (*p) (const void *) )

    ?

> * Define a macro _GL_UNUSED_ARGNAME(NAME) that expands to NAME if not 
> draft C2x, empty otherwise. Programs can use this macro for complicated 
> argument types where _GL_UNUSED_ARG does not suffice, e.g., 
> '_GL_maybe_unused int (*_GL_UNUSED_ARGNAME (p)) (int)'.

I object for two reasons:
  - it's a macro with type parameter (see above),
  - its purpose is just to make use of a misfeature in the language (see
    above as well).

> * Remove the snippet/unused-parameter module as it's not used now.

Indeed, this module is unused in gnulib. It may be used in packages that
use gnulib; therefore I vote for marking it 'obsolete' and remove it only
in a year or two.

Bruno



  reply	other threads:[~2021-08-08  1:41 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-31 19:52 Using C2x attributes more effectively in Gnulib Paul Eggert
2021-08-08  1:40 ` Bruno Haible [this message]
2021-08-09 10:08   ` Paul Eggert
2021-08-22 13:33     ` Bruno Haible
2021-08-22 20:05     ` gnulib-common.m4: Use C2x [[maybe_unused]] when possible Bruno Haible
2021-08-23  1:06     ` snippet/unused-parameter: Mark unused module obsolete Bruno Haible

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.gnu.org/mailman/listinfo/bug-gnulib

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=37018291.SK6AWJdEcI@omega \
    --to=bruno@clisp.org \
    --cc=bug-gnulib@gnu.org \
    --cc=eggert@cs.ucla.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).