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

Improve checking for GCC >= 8 and Clang for passing -Wno-class-memaccess #3598

Merged
merged 1 commit into from
Dec 22, 2022

Conversation

mstorsjo
Copy link
Contributor

@mstorsjo mstorsjo commented Dec 8, 2022

Don't pass -Wno-class-memaccess to Clang, which doesn't support that option name. (When inspecting $(CXX) with a pattern, "clang++" also matches the pattern "%g++".)

Simplify the pattern checking; use $(filter %g++,$(CXX)) instead of $(patsubst %g++,,$(CXX)) making it less obscure. (This requires changing the ifeq ($(),) into ifneq.)

Check for %clang++ in addition to %g++.

Also use a similar pattern for checking for clang++ instead of plain exact matches for "clang++", for passing the option -Wc++11-compat-reserved-user-defined-literal - this allows applying the option when CXX contains a path, or if "clang++" is prefixed by a cross triple.

@GuangweiWang
Copy link
Collaborator

LGTM

@mstorsjo
Copy link
Contributor Author

All the failures in CI are in ThreadDecodeFile/ThreadDecoderOutputTest.CompareOutput/*, which fails occasionally independent of these changes.

I'm considering if we should entirely ignore the ThreadDecodFile tests in the CI setup, since they're not passing deterministically. That would lose test coverage of them (instead of being mostly-working but failing occasionally, they could break entirely), but currently they're causing a lot of noise.

(Also, why can't the threaded decoding be fixed so that it deterministically always produces the correct reference output, instead of randomly producing different results?)

@huili2
Copy link
Collaborator

huili2 commented Dec 21, 2022

@xiaotiansf now this series of UT greatly breaks our CI. Do you think it's easy to fix or we'd disable them first?

@xiaotiansf
Copy link
Contributor

xiaotiansf commented Dec 21, 2022

I think it is safer to disable it in UT (I noticed there is two test cases 29 and 30 which generate different hash) for now until I find time to look at and fix it.

Don't pass -Wno-class-memaccess to Clang, which doesn't support that
option name. (When inspecting $(CXX) with a pattern, "clang++" also
matches the pattern "%g++".)

Simplify the pattern checking; use $(filter %g++,$(CXX)) instead of
$(patsubst %g++,,$(CXX)) making it less obscure. (This requires
changing the ifeq ($(),) into ifneq.)

Check for %clang++ in addition to %g++.

Also use a similar pattern for checking for clang++ instead of plain
exact matches for "clang++", for passing the option
-Wc++11-compat-reserved-user-defined-literal - this allows applying
the option when CXX contains a path, or if "clang++" is prefixed by
a cross triple.
@mstorsjo
Copy link
Contributor Author

Now after rebasing past 8535525, CI is passing cleanly.

@huili2 huili2 merged commit d2f5b21 into cisco:master Dec 22, 2022
@mstorsjo mstorsjo deleted the gcc-warn-option branch December 22, 2022 09:33
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.

4 participants