We'll need to do something for portability here (since many major compilers don't have__attribute__((fallthrough)), eg MSVC).

A bit nasty, hmm.

But, I welcome the suggestion to allow this warning to be enabled on Clang. According tothis LLVM issue andthis Flex issue (which Clang devs commented on), Clang simply isn't going to allow suppression of fallthrough warnings via a comment, which (in my opinion) is rather uncompromising and unhelpful, given decades of industry-wide use of/* fallthrough */ comments.

Our CI buildsdo enable-Wimplicit-fallthough on GCC, and the build does succeed due to GCC's support for suppression comments.

Ithink what we want is:

• Inpcre2_util.h, please add:

/* We define this fallthrough macro for suppressing -Wimplicit-fallthrough warnings.Clang only allows this via an attribute, whereas other compilers (eg. GCC) match attributesand also specially-formatted comments.For maximum portability, please use this macro as:  PCRE2_FALLTHROUGH /* Fall through */#if defined(__has_attribute)#if__has_attribute(fallthrough)#definePCRE2_FALLTHROUGH __attribute__((fallthrough));#endif#endif#ifndefPCRE2_FALLTHROUGH#definePCRE2_FALLTHROUGH#endif

• Use that syntax throughout, to work with GCC and Clang and any other compilers (there are dozens that people use with PCRE2...)

• Two of the YAML files in.github/ have a definition forCFLAGS_GCC_STYLE. We should update it to add-Wimplicit-fallthrough. It's included in-Wextra on GCC, but not on Clang.

Thank you!

Ah sorry, I was too quick changing stuff. I've introduced a similar check as for HAVE_ATTRIBUTE_UNINITIALIZED. Would that be okay as well?

Well done! Sorry we were working simultaneously there.

Users often pull files from PCRE2 into their own build system, so I try and keep use of Autoconf and CMake minimal. We probably have too many configure-time checks already.

The changes you made were not wrong... But I think I slightly prefer the approach I suggested!

Using__has_attribute will work with more build systems. I think every compiler with__attribute__((fallthrough)) will also have__has_attribute (it's only GCC and LLVM), so coverage should equally good.

(Interestingly, our GCC build on CI showschecking for __attribute__((fallthrough))... no andPerforming Test HAVE_ATTRIBUTE_FALLTHROUGH - Failed with your change.)

Okay, then I'd revert that last change and go with your approach.

Thank you for your understanding!

In my comment, I wrote-Wno-implicit-fallthrough in my suggested code snipped, but I've realised it's actually-Wimplicit-fallthrough. Apologies

Thank you for your understanding!

In my comment, I wrote-Wno-implicit-fallthrough in my suggested code snipped, but I've realised it's actually-Wimplicit-fallthrough. Apologies

I also had to change the/* Fall through */ to// Fall through // to avoid:
src/pcre2_util.h:136:21: error: '/*' within block comment [-Werror,-Wcomment]

Oh wait, actually the JIT build needs fixing :(

Mh, I wonder why we get:

src/pcre2_util.h:140:27: error: ISO C does not support '[[]]' attributes before C23 [-Werror=pedantic]  140 | #define PCRE2_FALLTHROUGH [[fallthrough]]

as I thought__has_c_attribute would check for that (seehttps://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fc_005fattribute.html). Mh?

Oh wait, actually the JIT build needs fixing :(

Mh, I wonder why we get:

src/pcre2_util.h:140:27: error: ISO C does not support '[[]]' attributes before C23 [-Werror=pedantic]  140 | #define PCRE2_FALLTHROUGH [[fallthrough]]

as I thought__has_c_attribute would check for that (seehttps://gcc.gnu.org/onlinedocs/cpp/_005f_005fhas_005fc_005fattribute.html). Mh?

Apparently not. Perhaps:

#if defined(__has_c_attribute)#if__has_c_attribute(fallthrough)&&__STDC_VERSION__ >=202311L#definePCRE2_FALLTHROUGH [[fallthrough]]#endif#endif

It's apparently an error to use the[[fallthrough]] syntax if the compiler isn't using--std=c23. Fussy, but fair enough.

Ah, for the clang compile we get:

src/pcre2grep.c:2058:3: error: [[]] attributes are a C23 extension [-Werror,-Wc23-extensions] 2058 |   PCRE2_FALLTHROUGH; /* Fall through */

I guess we cannot have-Wc23-extensions as[[fallthrough]] is a C23 extension.
Can this be modified somehow, or should I rather stick with the original approach?

Ah, for the clang compile we get:

src/pcre2grep.c:2058:3: error: [[]] attributes are a C23 extension [-Werror,-Wc23-extensions] 2058 |   PCRE2_FALLTHROUGH; /* Fall through */

I guess we cannot have-Wc23-extensions as [[fallthrough]] is a C23 extension. Can this be modified somehow, or should I rather stick with the original approach?

yes you can, but need to check for the C version that is being used first, see my comment onSTDC_VERSION

Okay, I've extended the check accordingly. I also replaced[[fallthrough]] by[[__fallthrough__]] as hinted above.

Ah right, I forgot about the JIT support... this requires a few additional changes. Working on it...

But it won't be as easy as you hoped. Because one of the source files is in another repo! 😢

I guess we're going to have to make a (very similar) PR intozherczeg/sljit to do the same whole business over there.

But it won't be as easy as you hoped. Because one of the source files is in another repo! 😢

I guess we're going to have to make a (very similar) PR intozherczeg/sljit to do the same whole business over there.

Indeed... I hope this finds similar acceptance.

Zoltan will take it (I think). It's a pat of PCRE2 that's been split out into a re-usable project. You can define SLJIT_FALLTHROUGH next to SLJIT_ASSERT, same as you defined PCRE2_FALLTHROUGH in the same file as PCRE2_ASSERT.

Are the recent changes okay for you? If so I'd then do a similar pull request for the SLJIT repository.

Are the recent changes okay for you? If so I'd then do a similar pull request for the SLJIT repository.

Yes. I won't randomise you on any cosmetic changes - it's perfectly fine as-is.

I'd be grateful if you did do the change in sljit.

Are the recent changes okay for you? If so I'd then do a similar pull request for the SLJIT repository.

Yes. I won't randomise you on any cosmetic changes - it's perfectly fine as-is.

I'd be grateful if you did do the change in sljit.

The pull request has been placed:

zherczeg/sljit#329

--

One thing I still need to investigate is the failure for themanyconfig run:

In file included from src/pcre2_internal.h:2393,                 from src/pcre2_study.c:46:src/pcre2_study.c: In function 'set_start_bits':src/pcre2_util.h:147:27: warning: a label can only be part of a statement and a declaration is not a statement [-Wpedantic]  147 | #define PCRE2_FALLTHROUGH __attribute__((__fallthrough__))       |                           ^~~~~~~~~~~~~src/pcre2_study.c:1805:7: note: in expansion of macro 'PCRE2_FALLTHROUGH' 1805 |       PCRE2_FALLTHROUGH; /* Fall through */      |       ^~~~~~~~~~~~~~~~~

One thing I still need to investigate is the failure for themanyconfig run:

In file included from src/pcre2_internal.h:2393,                 from src/pcre2_study.c:46:src/pcre2_study.c: In function 'set_start_bits':src/pcre2_util.h:147:27: warning: a label can only be part of a statement and a declaration is not a statement [-Wpedantic]  147 | #define PCRE2_FALLTHROUGH __attribute__((__fallthrough__))       |                           ^~~~~~~~~~~~~src/pcre2_study.c:1805:7: note: in expansion of macro 'PCRE2_FALLTHROUGH' 1805 |       PCRE2_FALLTHROUGH; /* Fall through */      |       ^~~~~~~~~~~~~~~~~

That's nasty. That's a very vexing parse!

The compiler sees:

case 1:__attribute__((fallthrough));case 2:

The problem is that the attribute attached to the "null statement" is not actually parsed as a statement unless there's a statement before it. (Yeah, C...) It's ambiguous - because it could be a declaration of a variable (which isn't a legal target for a case label). To disambiguate, it's treated as a null statement only if it follows another statement ("statement context").

Here's the solution...

caseOP_NCLASS:#if definedSUPPORT_UNICODE&&PCRE2_CODE_UNIT_WIDTH==8if (utf)        {re->start_bitmap[24] |=0xf0;/* Bits for 0xc4 - 0xc8 */memset(re->start_bitmap+25,0xff,7);/* Bits for 0xc9 - 0xff */        }#elifPCRE2_CODE_UNIT_WIDTH!=8SET_BIT(0xFF);/* For characters >= 255 */#else      ;/* Ensure case label interacts correctly with PCRE2_FALLTHROUGH#endif      PCRE2_FALLTHROUGH; /* Fall through */

Here's the solution...

#else      ;/* Ensure case label interacts correctly with PCRE2_FALLTHROUGH */#endifPCRE2_FALLTHROUGH;/* Fall through */

note, that it was missing a closing*/ in the added "null statement", so it should look instead like the one above.

I have been entertained by watching this saga roll by ... it just goes to show how complicated the world has become, with multiple compilers and multiple standards. I could never have imagined this when I first wrote PCRE. One comment for the record: the // comment is not new, not even "new". BCPL has had it from its birth in the 1960s, pre-dating C. I always wondered why // got dropped when the BCPL => B => C evolution happened. (Another thing they dropped was "endcase", overloading "break" instead, which is sometimes a right nuisance.)

You can partly blame C++. There's a lot of "enterprise" tooling in that area, which gradually trickles down to C as well, although the C folks try very hard to keep alive the bygone era of simple stuff.

One thing I still need to investigate is the failure for themanyconfig run:

In file included from src/pcre2_internal.h:2393,                 from src/pcre2_study.c:46:src/pcre2_study.c: In function 'set_start_bits':src/pcre2_util.h:147:27: warning: a label can only be part of a statement and a declaration is not a statement [-Wpedantic]  147 | #define PCRE2_FALLTHROUGH __attribute__((__fallthrough__))       |                           ^~~~~~~~~~~~~src/pcre2_study.c:1805:7: note: in expansion of macro 'PCRE2_FALLTHROUGH' 1805 |       PCRE2_FALLTHROUGH; /* Fall through */      |       ^~~~~~~~~~~~~~~~~

That's nasty. That's a very vexing parse!

The compiler sees:

case 1:__attribute__((fallthrough));case 2:

The problem is that the attribute attached to the "null statement" is not actually parsed as a statement unless there's a statement before it. (Yeah, C...) It's ambiguous - because it could be a declaration of a variable (which isn't a legal target for a case label). To disambiguate, it's treated as a null statement only if it follows another statement ("statement context").

Here's the solution...

caseOP_NCLASS:#if definedSUPPORT_UNICODE&&PCRE2_CODE_UNIT_WIDTH==8if (utf)        {re->start_bitmap[24] |=0xf0;/* Bits for 0xc4 - 0xc8 */memset(re->start_bitmap+25,0xff,7);/* Bits for 0xc9 - 0xff */        }#elifPCRE2_CODE_UNIT_WIDTH!=8SET_BIT(0xFF);/* For characters >= 255 */#else      ;/* Ensure case label interacts correctly with PCRE2_FALLTHROUGH#endif      PCRE2_FALLTHROUGH; /* Fall through */

Mh, I see. Yet, here I'd rather move the fallthrough statement into the#if/#elif parts. Just adding an empty statement looks odd to me.

I've closed my original pull requestzherczeg/sljit#329.@carenas has refined that stuff inzherczeg/sljit#330 which is required before this one can finally be merged (unless further changes are required).

Given thatzherczeg/sljit#330 has been merged can you give the recent changes another try? Or do you expect further changes to be done?

Thank you for the reminder. I was away for a few days and didn't check in on this.

I will do an update of the sljit component (#791) and then we can merge this.

If there any tweaks I wish to make, I'll just push them to this branch and not trouble you with them.

Many thanks again@Gsus42. I don't know how this turned into such an epic thread. That's not the normal process for contributions in PCRE2! Thank you for your patience.

Many thanks for having this change! Indeed this was much larger than I imagined :).