Combined -Wextra -Werror Commits #627
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
ghost
requested review from
bwbarrett,
rajachan,
rauteric and
a team
as code owners
ghost
force-pushed
the
wextra-werror
branch
from
797220f to
3369a2e
Compare
|
bot:aws:retest |
ghost
force-pushed
the
wextra-werror
branch
from
3369a2e to
13c78b9
Compare
bwbarrett
previously approved these changes
Sep 27, 2024
ghost
force-pushed
the
wextra-werror
branch
2 times, most recently
from
6ec3ccd to
b45d7e8
Compare
|
bot:aws:retest |
ghost
force-pushed
the
wextra-werror
branch
3 times, most recently
from
cb9dc75 to
ba70d3f
Compare
This was referenced Sep 30, 2024
Closed
ghost
force-pushed
the
wextra-werror
branch
from
ba70d3f to
36d0638
Compare
added 11 commits
September 30, 2024 10:34
c++ suppoorts initializers anywhere in the function, but one must not jump over an initializer with any goto usage. Given the lack of RAII in C, this becomes a significant painpoint. In large to-be-eventually-refactored functions contain gotos or use switch statements, split declaration and initialization, and move all declarations to the top of the function. This makes switch statements and gotos safe in both languages. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
typeof stopped working in newer GCC when in cxx mode. __typeof__ works in all cases and is the preferred solution for doing this portably. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
cpp requires explicit cast to double here. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
mr_key_size(), round_robin_threshold(), and eager_max_size() are strictly positive integers, but due to how we access them through our environment variable parameter macros, they are accessed as signed ints. When we eventually want to compare this against `size_t`s and other unsigned types, this creates sign comparison warnings. Just duplicate the macro so we can define unsigned environment params, and then move them to the unsigned macro. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Previously, this used ~0 as the invalid signal. This defaults to a signed type, which breaks under -wsign-compare. Prefer to use COMM_ID_INVALID (defined as COMM_ID_MASK) as the marker. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
const variables have internal linkage by default under c++; nvidia interface requires it not be const because its modified during init. Remove const for neuron, too. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
added 5 commits
September 30, 2024 10:34
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
commit ce214aa rearranged fields such that the written initializers were valid on most modern compilers, but it went unnoticed that this is insufficient for AL2's ancient toolchain, which fails with: > sorry, unimplemented: non-trivial designated initializers not supported provide all members for all entries to fix this. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Yet another autotools fix: If any warning/devel CFLAGS would invoke warnings in headers used to detect dependencies, those dependencies will fail with a highly misleading/confusing error message: > configure: Found .git directory. Adding -Werror to CFLAGS. > checking if running on EC2 instance... yes > checking if want AWS platform optimizations... yes > checking for Libfabric 1.18.0 or later... no > configure: error: On AWS platforms, Libfabric 1.18.0 or later is required ie: it is not that Libfabric 1.18 was not found, it was that its headers produced warnings. Fix this by resolving all dependencies before modifying CFLAGS. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
ghost
force-pushed
the
wextra-werror
branch
from
36d0638 to
065cd63
Compare
|
bot:aws:retest |
bwbarrett
approved these changes
Oct 1, 2024
|
bot:aws:retest |
ghost
mentioned this pull request
ghost
pushed a commit
that referenced
this pull request
Oct 9, 2024
We get this for free, likely fixed in #627 or with some of the -fanalyzer fixes. Enable -Wnull-dereference whenever -Werror is enabled. Signed-off-by: Nicholas Sielicki <nslick@amazon.com>
This pull request was closed.
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.
CI is moving too slowly, and rebasing triggers too many rebuilds, to continue to use stacked PRs. This is a single PR that is combining all of the following PRS:
For and PR marked above as unapproved, please click and re-review it. It will land (but will not be targeting master). Once all commits are "merged" into a some non-master branch, then we can just approve this PR, containing roughly the same contents, and we can merge it to master all-together and delete the branches.
I would prefer to be able to just land directly into master by changing base branches, but it's not realistic today given the way that CI is triggering. So here we are.
Description of changes:
Bunch of changes that bring us significantly closer to building successfully with C++, but also enables
-Werror -Wextragoing forward, and CI actions ensure that it will stay that way. I'm not sure that this is all pointless and nit-picky, a build failure caught on #624 very possibly can explain a segfault that was reported when initializing the plugin when less than 32 EFAs are available.edit: needed to be rebased due to a bug introduced during rebase on the very first in the series, see comment here.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.