Compile Under -pedantic -Werror and -std=c90
#2099
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
felixhandte
changed the title
-pedantic -Werror-pedantic -Werror and -std=c90
|
Looks like the failing test is just noise, but this looks good to me once all the CI is passing! |
|
We will need to compile with |
|
The proposed solution looks good to me. |
terrelln
approved these changes
May 2, 2020
Even without `-pedantic`, these macros will now fail to compile unless you provide an info string argument. This will prevent us from regressing.
`-Wall` implies `-Wformat-zero-length`, which will cause compilation to fail under `-Werror` when an empty string is passed as the format string to a `printf`-family function. This commit moves us back to prefixing the provided format string, which successfully avoids that warning. However, this removes the failure mode where that `RAWLOG` invocation would fail to compile when no format string was provided at all (which was desirable to avoid having code that would successfully compile normally but fail under `-pedantic`, which *does* require that a non-zero number of args are provided). So this commit also introduces a function which does nothing at all, but will fail to compile if not provided with at least one argument, which is a string. This successfully links the compilability of pedantic and non-pedantic builds.
Previously we would use it for all gcc-like compilations, even when a restrictive mode that disallowed it had been selected.
It was causing build issues in ANSI mode.
It's complaining about the `memcpy`s, saying: "warning C4090: 'function': different 'const' qualifiers" Let's try explicitly casting to the argument types...
felixhandte
force-pushed
the
compile-under-pedantic
branch
from
56bcf6e
to
2cf72d5
Compare
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This changeset allows Zstd to be built without warnings under
-pedantic. Specifically, this was tested with:This addresses #2035. See also previous threads on this topic: #1989, #1540, and #1538.
It also lets Zstd build successfully (though not without warnings) under C90/C89/ANSI mode.
This PR accomplishes this in a different way than previous solutions. Rather than diverge into two kinds of macros with and without additional arguments, it simply complies with C99 by making sure every invocation of these macros does have at least one argument.
Known deficiencies:
RETURN_ERROR_IF,RETURN_ERROR, andFORWARD_IF_ERRORmust have an info string.Why I selected this approach: adding
, ""to the args is no more characters than adding_MSGto the name of the macro, and I'd rather bias contributors towards including explanatory text than away.That being said, we could also introduce versions of these macros that don't take additional arguments. I'm open to suggestions.