Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: fix -Wsign-compare warnings #430

Merged
merged 1 commit into from
Oct 13, 2024
Merged

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Oct 7, 2024

Projects being rewritten and compiled may have warnings/errors like this turned on, so it's simpler to just fix them.

Note that this doesn't set -Werror=sign-compare in cmake because that'll apply to all of the tests, too. We just want to make sure that included headers work under different possible warning/error configurations a project might set.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 7, 2024

Maybe we should build with -Wsign-compare as well? I mean we already build with ubsan to catch potential issues.

@kkysen
Copy link
Contributor Author

kkysen commented Oct 7, 2024

Maybe we should build with -Wsign-compare as well? I mean we already build with ubsan to catch potential issues.

Sure, we can do that, too. I think the -Wsign-compare came from something like -Wall so we should probably just do that.

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 7, 2024

-Wall would be nice but it's not a priority so let's stick to just -Wsign-compare for now. Also when we update the docs we should keep track of which flags are required and which are just nice-to-haves.

@rinon rinon requested a review from ayrtonm October 7, 2024 20:43
Copy link
Contributor

@ayrtonm ayrtonm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after adding -Wsign-compare here.

@kkysen kkysen requested a review from ayrtonm October 8, 2024 08:44
@kkysen
Copy link
Contributor Author

kkysen commented Oct 8, 2024

@ayrtonm, hmm, now I'm hitting other sign-compares in the other examples/tests. Is there a way I can only enable -Wsign-compare in cmake some of the time, because this is something that we want to work without errors no matter what flags the downstream user is setting. Maybe it's easier to just not set -Werror=sign-compare for us?

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 8, 2024

Sorry that was for all compartments. This is probably where we want the flag so it'll only apply to libia2.a https://github.com/immunant/IA2-Phase2/blob/main/runtime/libia2/CMakeLists.txt#L5

@ayrtonm
Copy link
Contributor

ayrtonm commented Oct 8, 2024

Actually the last place I linked wouldn't apply it to the original places you fixed, only the sources in runtime/libia2 so I'm fine leaving the flag out for now.

Projects being rewritten and compiled may have warnings/errors like this turned on,
so it's simpler to just fix them.

Note that this doesn't set `-Werror=sign-compare` in `cmake`
because that'll apply to all of the tests, too.
We just want to make sure that included headers work under
different possible warning/error configurations a project might set.
@kkysen kkysen force-pushed the kkysen/fix-Wsign-compare-warning branch from b8b8046 to a87ec12 Compare October 13, 2024 03:40
@kkysen
Copy link
Contributor Author

kkysen commented Oct 13, 2024

Merging now since @ayrtonm already approved this version and we decided not to set -Werror=sign-compare anymore.

@kkysen kkysen merged commit 872cdfd into main Oct 13, 2024
34 checks passed
@kkysen kkysen deleted the kkysen/fix-Wsign-compare-warning branch October 13, 2024 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants