-
Notifications
You must be signed in to change notification settings - Fork 75
fix: add -Wno-vla-extension (note, -Wno-vla-cxx-extension might be needed in the future) #1439
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
Conversation
✅ COMPAS Build Successful!
Detailed Evolution PlotGenerated by COMPAS CI |
… Wno-vla-extension
|
Hey @jeffriley -- lets just foget about the changing flag for clang>18 for now? Ive left a comment in case this becomes an issue in the future. |
✅ COMPAS Build Successful!
Detailed Evolution PlotGenerated by COMPAS CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @avivajpeyi !
A few questions:
Why have you added -Wno-sign-compare? I think if we are generating these warnings we should fix the code - I know sometimes it's easier just to ignore the warning (or turn it off), but comparing a signed int to an unsigned (even size_t) is a bit of a code smell imo.
You've added -Wno-unused-variable - we should remove any unused variables, not just ignore their presence.
You've added -Wno-unused-parameter. Are you seeing any of these warnings (I'm not with gcc on linux)? For C++17 and later we can add the [[maybe_unused]] attribute. Not sure if that's available in clang (but I suspect it would be).
Ditto for -Wno-unused-function - although tbh we should remove any unused functions - if we find we need them later we can reinstate them from github.
You've added -Wno-reorder-ctor. Are you seeing any of these warnings (I'm not with gcc on linux)? If you are, we should fix the code - that's a bit of a code smell imo, and could cause problems in the future if there's a dependency on member initialisation.
Removed unnecessary warning suppressions from EXTRA_WARN_SUPPRESS.
|
OK -- lets just remove the extra warning-surpressment |
jeffriley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great - that works :-) Thanks!
✅ COMPAS Build Successful!
Detailed Evolution PlotGenerated by COMPAS CI |



@SimonStevenson is getting some new warnings when building
I think we need different flags based on clang versions....
Clang < 17 → -Wno-vla-extension
Clang ≥ 17 → -Wno-vla-cxx-extension
(for context -- my Jetbrains IDE is what suggested me to use
Wno-vla-cxx-extensionin place ofWno-vla-extension