Fix some clang++ warnings

[TedLyngmo]

Compiled with -Weverything -Wno-c++98-compat -Wno-c++98-compat-pedantic and fixed some low hanging fruits.

[biojppm]

Thanks for the PR, it is really appreciated.

First, some context: -Weverything is not actually advised by clang.

There are a couple of changeset remarks/questions above. Other than these, there are a few others:

• the initial diff had a rather large part fixing leading underscores in some symbols; subsequently they're not here. Why the revert?
• Also, on this same subject, I think suppressing the warning as needed with file-wide push/pop is not just lower effort, but might actually be preferable. IMO there's really low risk of having these symbols clash with any other symbol.
• what are the final compile flags that you're looking to suppress? I think adding these to the CI is in order, but I'm a bit confused as to what the flags are based on the PR description and commit error messages.
• related - were there no warnings coming from c4core code? I would really expect that to be the case, but there's no PR there.

[TedLyngmo]

First, some context: -Weverything is not actually advised by clang. But I'm really inclined

Yes, using it means you have no control over the warnings that will be enabled. Every new version of clang++ will come with new warnings. I use it as a means to enable everything in the current version of clang++ that I have - and then remove it once I've fixed everything that popped up and looked interesting enough :) Shipping a library with -Weverything enabled is not a good idea.

There are a couple of changeset remarks/questions above. Other than these, there are a few others:

• the initial diff had a rather large part fixing leading underscores in some symbols; subsequently they're not here. Why the revert?

I misunderstood the large impact and concentrated on just being able to compile the library clean. As it happens, CI showed that there is a lot more code that depends on those identifiers :-) I removed that commit from the set and was thinking of addressing that separately now that I have a better picture of how they are used.

• Also, on this same subject, I think suppressing the warning as needed with file-wide push/pop is not just lower effort, but might actually be preferable. IMO there's really low risk of having these symbols clash with any other symbol.

True, but there are organizations that are sticklers when it comes to undefined behavior and they'd probably reject the library when noticing these identifiers. I have seen header guards and other reserved identifiers defined in libraries collide with the implementation's own identifiers after upgrading the toolchain. Not a nice surprise.

• what are the final compile flags that you're looking to suppress? I think adding these to the CI is in order, but I'm a bit confused as to what the flags are based on the PR description and commit error messages.

I wasn't thinking of suppressing any. Possibly adding an explicit set of warnings after having fixed those specific issues.

• related - were there no warnings coming from c4core code? I would really expect that to be the case, but there's no PR there.

Oh, there were tons. Since it's in a submodule, I left those untouched. It'd probably be a good idea to go through those separately.

[biojppm]

what are the final compile flags that you're looking to suppress? I think adding these to the CI is in order, but I'm a bit confused as to what the flags are based on the PR description and commit error messages.

I wasn't thinking of suppressing any. Possibly adding an explicit set of warnings after having fixed those specific issues.

My bad -- I meant fix instead of suppress. The question still stands. What are those warnings? The PR description says one thing but individual commits show another.

[TedLyngmo]

what are the final compile flags that you're looking to suppress? I think adding these to the CI is in order, but I'm a bit confused as to what the flags are based on the PR description and commit error messages.

I wasn't thinking of suppressing any. Possibly adding an explicit set of warnings after having fixed those specific issues.

My bad -- I meant fix instead of suppress. The question still stands. What are those warnings? The PR description says one thing but individual commits show another.

Each commit is supposed to only fix the warning(s) mentioned in the four individual commit messages. For the PR, it could say "Fix clang++ warnings: -Wmissing-noreturn, -Wextra-semi, -Wextra-semi-stmt, -Wimplicit-fallthrough and -Wnrvo" but that'd require updating the PR message if I pull anything out of the set (or add anything to it).

[biojppm]

Looking at the failing windows tests : the newly-added C4_NORETURN to the tree handler events now causes a warning C4702: unreachable code on MSVC.

This is triggered from an engine test that checks for an error (ie requires an error) in the specified sequence of event handler calls when using the tree handler. That test must remain.

IMO the way to tackle this is to work around it by suppressing that warning it in the offending test source code: adding C4_SUPPRESS_WARNING_MSVC_WITH_PUSH(4702) and corresponding _POP at the top/bottom of the files.

AFAICT these are the files where that would be needed:

• test/test_engine_3_map_block_2.cpp
• test/test_engine_3_map_flow_1.cpp
• test/test_engine_4_anchor.cpp
• test/test_engine_5_tag_1.cpp
• test/test_engine_6_qmrk_1.cpp
• test/test_engine_6_qmrk_2.cpp
• test/test_engine_6_qmrk_3.cpp
• test/test_engine_6_qmrk_4.cpp
• test/test_engine_7_seqimap.cpp

HTH.

[biojppm]

The failure in the coverage workflows is unrelated with this PR, and can be ignored. It's a token access problem that must be fixed elsewhere.

The install singleheader failure is a flaky test ; also unrelated.

[biojppm]

I pushed a bunch of branches before approving this run. It's going to take a while.

[biojppm]

Thanks!