[v3,0/3] -Wunreachable-code

Message

Junio C Hamano March 17, 2025, 11:53 p.m. UTC

As Taylor noticed, we can still help macOS users by first dealing
with the false positive in the code, and then flip the warning
option for developers on.

 [1/3] run-command: use errno to check for sigfillset() error

 This was our first "workaround" that is very specific to the code
 that gets falsely flagged by the compiler.

 [2/3] git-compat-util: add NOT_CONSTANT macro and use it in atfork_prepare()

 This adds a more generic way to work around a false positive from
 -Wunreachable-code to prevent compilers from optimize away
 expressions that are used in conditionals, and rewrite the earlier
 workaround with it.

 [3/3] config.mak.dev: enable -Wunreachable-code

 Now we worked around known false positive of -Wunreachable-code,
 we force it upon our developers, including macOS ones.

This is totally offtopic, but I often find the short-log (list of
commits, grouped by author) in the cover letter very awkward to work
with.  Between v2 and v3, aside from the NOT_CONSTANT() improvements
in the patch [2/3] that used to be [3/3], one large change is the
reordering of the patches but that is not seen in the shortlog (I
ran "git log --oneline -reverse" to prepare the list of commits in
the order they are applied to describe them in the above list).

Jeff King (2):
  run-command: use errno to check for sigfillset() error
  config.mak.dev: enable -Wunreachable-code

Junio C Hamano (1):
  git-compat-util: add NOT_CONSTANT macro and use it in atfork_prepare()

 Makefile                         | 1 +
 compiler-tricks/not-a-constant.c | 2 ++
 config.mak.dev                   | 1 +
 git-compat-util.h                | 9 +++++++++
 meson.build                      | 2 ++
 run-command.c                    | 8 +++++++-
 6 files changed, 22 insertions(+), 1 deletion(-)
 create mode 100644 compiler-tricks/not-a-constant.c

Comments

Jeff King March 18, 2025, 12:18 a.m. UTC | #1

On Mon, Mar 17, 2025 at 04:53:26PM -0700, Junio C Hamano wrote:

> As Taylor noticed, we can still help macOS users by first dealing
> with the false positive in the code, and then flip the warning
> option for developers on.

Yeah, this is worth doing.

> This is totally offtopic, but I often find the short-log (list of
> commits, grouped by author) in the cover letter very awkward to work
> with.  Between v2 and v3, aside from the NOT_CONSTANT() improvements
> in the patch [2/3] that used to be [3/3], one large change is the
> reordering of the patches but that is not seen in the shortlog (I
> ran "git log --oneline -reverse" to prepare the list of commits in
> the order they are applied to describe them in the above list).
> 
> Jeff King (2):
>   run-command: use errno to check for sigfillset() error
>   config.mak.dev: enable -Wunreachable-code
> 
> Junio C Hamano (1):
>   git-compat-util: add NOT_CONSTANT macro and use it in atfork_prepare()

The re-ordering does appear in the range-diff, if you provide one. But I
agree that the organized-by-name shortlog does not make much sense for
most series. As a reviewer, I care most about the patches, not the
authors.

I make my cover letters with something like this (part of a larger
script):

    git format-patch --stdout origin..$topic |
    perl -lne '
      if (/^Subject: (.*)/) {
        $subject = $1;
      }
      elsif ($subject && /^\s+(.*)/) {
        $subject .= " $1";
      }
      elsif ($subject) {
        print $subject;
        $subject = undef;
      }
    ' |
    sed -e 's/\[PATCH /[/' \
        -e 's/]/]:/' \
        -e 's/^/  /'

which yields something like (for the older version of this series):

  [1/3]: config.mak.dev: enable -Wunreachable-code
  [2/3]: run-command: use errno to check for sigfillset() error
  [3/3]: git-compat-util: add NOT_A_CONST macro and use it in atfork_prepare()

Having the correct order and the matching numbering next to each one
makes it much easier if you're going to comment on them inline.

The perl in the script above is required to handle rfc822 wrapping /
line continuation. I never bothered to implement rfc2047 unquoting. I
don't tend to use non-ascii chars in my subject lines. ;)

It would be nice if we had a format-patch option to avoid quoting and
wrapping in order to make text processing like this easier.

-Peff