Flex should use '__attribute__((fallthrough))' instead of /*FALLTHROUGH*/

[davidbolvansky]

Please see: https://bugs.llvm.org/show_bug.cgi?id=43465

[Explorer09]

I've read the discussion in the page you linked, and I wonder: Is there any reason for clang not to support /* FALLTHROUGH */ comments?
The problem of __attribute__((fallthrough)) or [[fallthrough]] is that neither are portable, especially to older compilers. And we (not just Flex, but almost all existing C codebases around the Web) would need to implement a complex macro wrapper just to make code backward-compatible as well as to support new fallthrough syntax. GCC's detection of /* FALLTHROUGH */ comments is already a good compromise to the problem, so why couldn't clang do the same?

[davidbolvansky]

complex macro wrapper

Well, the code below looks okay for me, nothing complex. Put into into macro "FLEX_FALLTHRU" -> profit.

#if defined(__has_attribute)
#if __has_attribute(fallthrough)
      __attribute__((fallthrough)); 
#endif
#endif

[westes]

The code you suggest does not account for the current comment style of fallthrough.

[Explorer09]

@davidbolvansky Seriously your fallthrough code didn't address C++17 fallthrough syntax. I'll rather have a complete and backward compatible wrapper macro that can solve this once and for all, instead of a half-baked solution.

[davidbolvansky]

attribute((fallthrough) is universal way, no?

No warnings with it even in C 2X/C++17 mode.

[Explorer09]

@davidbolvansky __attribute__ is a gcc-invented syntax. Although gcc is popular and almost the default C/C++ compiler in Linux systems, it's not a technically "universal" syntax. Did you test on Microsoft's Visual C++ compilers or Intel C compilers? Those are most unlikely to support GNU C syntaxes, at least not until recently. By the way, you should not take clang's support for GNU syntaxes for granted, and that's why I disagree with your claim that __attribute__ is universal enough.

[davidbolvansky]

ICC:
icc: command line warning #10148: option '-Wimplicit-fallthrough' not supported

-Wall -Wextra does not warn at all in this case. So nothing to silence?

Applied

#if defined(__has_attribute)
#if __has_attribute(fallthrough)
      __attribute__((fallthrough)); 
#endif
#endif

ICC 13+ is fine with it.

MSVC with /Wall or /W4 does not warn too. And these lines above cause no issues for MSVC.

I didnt say you should just put __attribute__((fallthrough)) under macro. I wanted to say - put those lines above behind macro..

[Explorer09]

@davidbolvansky Actually I know what __has_attribute does. No need to explain. I am just not sure if going through all the trouble is worth it. Is there any chance that implicit fallthrough check would become mandatory in the C++ standard?
Note: I personally am not totally against compiler-specific keywords (I'm not flex maintainer, by the way), but I think if we are to introduce one, it should be worth it (e.g. I was thinking of introducing noreturn in flex before).
Also, since we are modifying the skeleton, where can we find the policy of what symbol names are reserved for flex internal, so that we won't clash with user code?

[davidbolvansky]

Actually I know what __has_attribute does. No need to explain.

You raised concern about other compilers, so I just went and checked it and wrote about the situation here.

Is there any chance that implicit fallthrough check would become mandatory in the C++ standard?

[[fallthrought]] is mandatory to be supported since C 20 / C++ 17.

[Explorer09]

@davidbolvansky

[[fallthrought]] is mandatory to be supported since C 20 / C++ 17.

I'm not talking about mandatory support of the attribute syntax; I'm talking about the implicit fallthrough warning and whether it would become mandatory.

[davidbolvansky]

No, compilers don't have to diagnose it, it is not required by C/C++ standard.

[Explorer09]

@davidbolvansky

No, compilers don't have to diagnose it, it is not required by C/C++ standard.

And that is one of the reason for flex not to adopt __attribute__((fallthrough)) for now. Like I said in the first comment, it would be better to wait for clang to support the /* fallthrough */ comment, than to change flex code just to workaround that problem specific to clang (and it's only the development versions of clang).

[AaronBallman]

@davidbolvansky

No, compilers don't have to diagnose it, it is not required by C/C++ standard.

And that is one of the reason for flex not to adopt __attribute__((fallthrough)) for now. Like I said in the first comment, it would be better to wait for clang to support the /* fallthrough */ comment, than to change flex code just to workaround that problem specific to clang (and it's only the development versions of clang).

FWIW, we have recently reconfirmed our decision not to support comments as a mechanism to suppress diagnostics in Clang. Also, I think it's incorrect to claim this is a problem specific to Clang (see below).

@davidbolvansky __attribute__ is a gcc-invented syntax. Although gcc is popular and almost the default C/C++ compiler in Linux systems, it's not a technically "universal" syntax. Did you test on Microsoft's Visual C++ compilers or Intel C compilers? Those are most unlikely to support GNU C syntaxes, at least not until recently. By the way, you should not take clang's support for GNU syntaxes for granted, and that's why I disagree with your claim that __attribute__ is universal enough.

[[]] is a standardized syntax in both C++ (since C++11) and C (starting in C2x). __attribute__ is not standardized, but it is supported by GCC, Clang, ICC, ICX, TCC, cproc, as well as other compilers. It's not ubiquitous, but it's also not uncommon to support.

Specific to the compilers you asked about: ICC supports many GNU extensions, as does ICX, including __attribute__((fallthrough)), but ICC does not have a flag to diagnose implicit fallthrough despite supporting the attribute. MSVC also doesn't diagnose implicit fallthrough except as part of their C++ Core Guideline static analysis checks, which comments do not suppress (at least no comments I've found will do it).

IMO, comments are the least portable way to silence this diagnostic (out of the list of compilers you've asked about, only GCC supports that). The most portable way is to use the utilities compilers have provided to query for support and then fall back to comments as a last resort. Something along these lines works significantly better than hoping comments are parsed for semantic effects:

// Boilerplate for compiler compatibility.
#ifndef __has_cpp_attribute
#define __has_cpp_attribute(x) 0
#endif

#ifndef __has_c_attribute
#define __has_c_attribute(x) 0
#endif

#ifndef __has_attribute
#define __has_attribute(x) 0
#endif

// Actually interesting bit to define the FALLTHROUGH macro.
#if __has_cpp_attribute(fallthrough) || __has_c_attribute(fallthrough)
#define FALLTHROUGH [[fallthrough]]
#elif __has_attribute(fallthrough)
#define FALLTHROUGH __attribute__((fallthrough))
// Note, there could be more branches here, like using `[[gsl::suppress]]` for MSVC, or
// other vendor-specific attributes/spellings. This list is not intended to be exhaustive.
#else
#define FALLTHROUGH
#endif

// Silly example code
void func(int something) {
  extern void foo(void), bar(void);
  switch (something) {
  default: break;
  case 0:
    foo();
    // Note the use of the comment -- that's a good fallback for the compilers that you can't
    // write a FALLTHROUGH macro for but still parse comments for diagnostic suppression.
    FALLTHROUGH; /* fallthrough */
  case 12:
    bar();
    break;
  }
}

You might want to reopen this issue to reconsider your stance on this. GCC added implicit fallthrough warnings to -Wextra. Clang has not yet done so, but we have had requests to do it and I can imagine a future where that happens. (I would be very surprised if either GCC or Clang enabled implicit fallthrough warnings by default though.)

[Explorer09]

@AaronBallman Although I can't represent the Flex maintainer who can decide on this issue, my opinion is that it's okay to implement a macro in Flex that can keep both GCC and Clang happy. Feel free to make a pull request if someone has the time to do it.
A few comments though:

1. I prefer the macro be called YY_FALLTHROUGH rather than just FALLTHROUGH. And I would like to see the macro also used in the generated lexers (the skeleton code). The YY prefix prevents the clashing with any user defined symbols (POSIX reserved this prefix for lex and yacc internal use).
2. I think with the macro the comment would seem redundant. Any compiler (or GCC in particular) that can detect the comment should also be able to support one of the attribute syntaxes. Would there be any counter example?

[AaronBallman]

@AaronBallman Although I can't represent the Flex maintainer who can decide on this issue, my opinion is that it's okay to implement a macro in Flex that can keep both GCC and Clang happy. Feel free to make a pull request if someone has the time to do it.

Thank you! I likely won't have the time to make a PR for this myself, just for full disclosure.

I prefer the macro be called YY_FALLTHROUGH rather than just FALLTHROUGH. And I would like to see the macro also used in the generated lexers (the skeleton code). The YY prefix prevents the clashing with any user defined symbols (POSIX reserved this prefix for lex and yacc internal use).

That seems perfectly reasonable to me.

I think with the macro the comment would seem redundant. Any compiler (or GCC in particular) that can detect the comment should also be able to support one of the attribute syntaxes. Would there be any counter example?

In terms of GCC, I think it likely isn't necessary to also have a comment there.

I had the impression from this thread that you were aware of other compilers that support diagnosing implicit fallthrough and silencing diagnostics through comments but not support an attribute syntax, which is why I suggested leaving the comment in place. I'm not personally aware of any such implementation in the wild, but there are a surprising number of C compilers in the world.

[westes]

it's okay to implement a macro in Flex that can keep both GCC and Clang happy.

Yes, especially if we're hiding platform specific details under the hood.

Any pr's for YY_FALLTHROUGH are quite welcome.

[Explorer09]

@AaronBallman

I had the impression from this thread that you were aware of other compilers that support diagnosing implicit fallthrough and silencing diagnostics through comments but not support an attribute syntax.

No actually. My impression was that the fallthrough comment would be a semi-standard way for suppressing the fallthrough warning, before a standard attribute syntax (for C++ or C) can be defined. It seems that it's no longer the case. The compilers in the wild either do not support the warning or are capable of reading any form (standard or GCC-like) of attribute syntax now.

[AaronBallman]

Ah excellent, that matches my understanding of the situation with compilers today. Then the comment is redundant and hopefully unnecessary.

[Explorer09]

I have one question regarding the flex skeleton: Since the skeletons use [[ and ]] internally as M4 quotes, How do I insert the literal [[ and ]] character for C/C++ attribute syntax? I mean, it looks ugly to escape the quote marks like this: [/**/[ ]/**/] and I wish a more pretty way of inserting the characters.

By the way, the C standard digraphs for [ and ] are <: and :>.

I'm not sure if my draft works, but here it is:

#if !defined(YY_FALLTHROUGH) && defined(__has_cpp_attribute)
#if __has_cpp_attribute(fallthrough) /* since C++17 */
#define YY_FALLTHROUGH <:<:fallthrough:>:>
#endif
#endif

#if !defined(YY_FALLTHROUGH) && defined(__has_c_attribute)
#if __has_c_attribute(fallthrough) /* since C23 */
#define YY_FALLTHROUGH <:<:fallthrough:>:>
#endif
#endif

#if !defined(YY_FALLTHROUGH) && defined(__has_attribute)
#if __has_attribute(fallthrough)
/* GNU-style attribute. The __has_attribute() macro was first available in
   Clang 2.9 and incorporated into GCC since GCC 9. */
#define YY_FALLTHROUGH __attribute__((__fallthrough__))
#endif
#endif

#if !defined(YY_FALLTHROUGH) && defined(__GNUC__) && defined(__GNUC_MINOR__)
/* The fallthrough attribute and "-Wimplicit-fallthrough" was introduced in
   GCC 7, before the support of __has_attribute(). */
#if __GNUC__ >= 7
#define YY_FALLTHROUGH __attribute__((__fallthrough__))
#endif
#endif

#if !defined(YY_FALLTHROUGH)
#define YY_FALLTHROUGH ((void)0)
#endif

[westes]

@Explorer09 try this:

https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.70/html_node/Quadrigraphs.html#Quadrigraphs