Introduce fallthrough as a wrapper around the fallthrough attribute

[MisterDA]

Recent compilers have introduced the -Wimplicit-fallthrough warning
against the C switch fallthrough. OCaml already uses the /* fallthrough */
C comment that GCC 7 and above recognize for some level
given to the warning, but the fallthrough statement is also available
since C++17 and either as a language statement (expected in C2X) or as
a compiler/language attribute.

This commit makes use of the statement/attribute when available, or
falls back to the fallthrough comment, and uses the new
CAMLfallthrough macro wherever the comment was used.

[MisterDA]

Found more cases of fallthrough to replace.
Shoud I probe the compiler for support of -Wimplicit-fallthrough in the configure script to enable it by default?
Note that in gcc, it's also part of -Wextra.

[xavierleroy]

I'm not sure what you're trying to achieve. For quite a while, gcc has been able to check switch statements based on the presence of /* fallthrough */ comments, plus the appropriate -Wimplicit-fallthrough flag. Why do you want to replace nice-looking /* fallthrough */ comments by scary-looking CAMLfallthrough macro and its scary-looking, #ifdef-fest definition? What is gained?

[MisterDA]

I agree that comments in general look nicer than the attributes or my
CAMLfallthrough proposal.

I think however that using the attributes/statement syntax has
advantages over the comments. Some of the reasons below are more
subjective than others, they may be of less relevance to you.

I'm unconfortable putting a semantic meaning into a comment. I think
they should be reserved to the programmer. Having the compiler peek
into them looks at worst flaky to me, at best the poor man's static
analysis. If we were to run the preprocessor separately from the
compiler, we'd have to tell it to preserve comments. This is also very
much dependent on the compiler. Clang doesn't read comments but
supports the attribute. The fallthrough statement has been added to
C++17 and it has been included in the draft of the next C
revision. Soon-ish we'll be able to make use of an actual language
feature rather than a compiler extension (or a compiler reading the
comment section).

I also don't find it obvious which level to use for the warning; in my
tests GCC 11.2 (mingw-w64) there always was at least one warning (in
globroots.c or interp.c) regardless of the level, even if there was a
variation on the fallthrough comment. Instead of fixing this comment
so that it can be recognized by a specific compiler, we could change
the code so that it can have the best relationship with all
compilers.

In interp.c I prefer Fallthrough; for the parallel with Next;
rather than variations on /* Fallthrough */.

Now, to get a better looking macro we could use simply fallthrough;
as done in the Linux kernel, and we could put it in a private header
not installed, to drop the caml prefix namespace. I think that
would also simplify the macro dance.

I haven't triggered any warning report for a forgotten fallthrough
comment, indicating that the code is well annotated and not in dire
need of this patch (but perhaps a slight adjustment to some comments),
so if you're not convinced I won't push for it further.

[MisterDA]

Rebased the PR to fix Changes conflicts.
Let me know what I can do to improve it, if needed.

[dra27]

This PR would have allowed us to avoid the slip in #11002 fixed in #11066. dra27@4882f3e adds the necessary support to configure to add -Wimplicit-fallthrough to the C warnings enabled during the build (I wish to atone for failing to spot the issue when I reviewed #11002, even if I did spot another bug...).

There is a case for the CAMLfallthrough macro vs having the comments: I believe clang supports -Wimplicit-fallthrough but doesn't recognise gcc's /* fall through */ markers.

[MisterDA]

This PR (or at least the warning) would have caught this new bug before it was introduced, maybe we could revisit it? Move the detection of the attribute to configure as not to clutter the headers?

[dra27]

runtime/interp.c creates a subtle problem if we just use the existing comments approach. I'm guessing that whatever GCC does with the comments must occur before pre-processing, which means that it doesn't realise that Instruct(ACC) is actually a case at the time it looks at the /* fallthrough */ comment. The warning therefore triggers on interp.c when compiled for the debug runtime (which is not threaded) regardless of the comments.

However, it turns out this can be fixed by including -CC (which keeps comments during pre-processing). I have a minimal dra27@f425a4e which adjusts just the two "fall through" comments which GCC doesn't recognise and turns the warning on for GCC only.

I think we should definitely merge one of these solutions, although my preference is for @MisterDA's, as it enables the warning for clang as well.

[MisterDA]

I've cleaned, simplified, and rebased this PR. Could this be reconsidered?

[MisterDA]

if you're not convinced I won't push for it further.

Evidently, I lied.

There is a case for the CAMLfallthrough macro vs having the comments: I believe clang supports -Wimplicit-fallthrough but doesn't recognise gcc's /* fall through */ markers.

That's the strongest point, I think.

I've rebased once more, and chosen to guard definition of fallthrough under CAML_INTERNALS, so that there's no need to namespace it.

[gasche]

I don't have a clear opinion or expertise on these topics, but I can see that @MisterDA would be happy to get some feedback so I had a look. This looks okay: I trust @dra27's positive gut feeling, and I agree with the sentiment that proper syntax is better than a comment. I don't see a cost to this PR besides the churn and potential for regression, and arguably some benefits, so I propose to go ahead and merge it eventually.