-
Notifications
You must be signed in to change notification settings - Fork 820
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
-Wshadow fixes #3517
-Wshadow fixes #3517
Conversation
…t as it's troubling example of shadowing of multiple variables at multiple levels.
@@ -96,11 +96,11 @@ class SpecConstantOpModeGuard { | |||
|
|||
struct OpDecorations { | |||
public: | |||
OpDecorations(spv::Decoration precision, spv::Decoration noContraction, spv::Decoration nonUniform) : | |||
precision(precision) | |||
OpDecorations(spv::Decoration in_precision, spv::Decoration in_noContraction, spv::Decoration in_nonUniform) : |
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.
I have a dislike for -Wshadow
because of this particular type of change. If we're going to enable shadow warning, would you find -Wshadow=local
or -Wshadow-compatible-local
acceptable?
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.
VulkanSceneGraph uses -Wshadow
, and it ends up affecting the nested glslang build, too. If glslang has problems with -Wshadow
, we'll have to keep jumping though the same hoops to avoid warning spam, even if the lesser shadowing warnings are still resolved.
VSG's own code avoids generating the warnings by prefixing member variables with _
, and if it were my own project, I'd use an m
prefix, which would be how I'd write things in an object-oriented language anyway even if there wasn't a warning enforcing it as it makes scope visible. This is generally a pretty widespread way of doing things, and lots of people prefer it, even if it's not quite universal and isn't specified in the C++ Core Guidelines.
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.
VulkanSceneGraph uses -Wshadow, and it ends up affecting the nested glslang build, too
Is this where glslang is included in the VulkanSceneGraph build? https://github.com/vsg-dev/VulkanSceneGraph/blob/d1c20da54722867c1cbdb5a7656e376a11f2edcf/src/vsg/CMakeLists.txt#L237.
Is there a reason find_package
is not used? Or maybe it is? I didn't see `build_vars.cmake. The flags used by VulkanSceneGraph should not affect glslang and vice-versa. I believe if you are using find_package with glslang this would not be an issue, but I'm not 100% sure; in any case, it would probably be a good idea to figure out why the flags set in VSG's project are affecting glslang's project.
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.
Basically because until glslang 14, the exported CMake configuration was busted (I've not even confirmed that 14 works everywhere, but all of the problems I know about are resolved by then), and most platforms don't have 14 yet. I've got some work locally that makes it use the system-wide install of 14 if it's present (and has an option to specify an earlier one if a package maintainer believes it to be fine for their platform), but it's not made it to the main project yet - I'd hoped to have building a relatively recent version of glslang as a fallback, as the old one currently used is incompatible with GCC 13 thanks to using <cstdint>
symbols without including the header, but I hit a slew of problems, hence my series of PRs here.
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.
If glslang has problems with
-Wshadow
To be clear, I wasn't saying "no" but I do have reservations. I'd like to know how @ncesario-lunarg and @arcady-lunarg feel about it.
This is generally a pretty widespread way of doing things, and lots of people prefer it, even if it's not quite universal and isn't specified in the C++ Core Guidelines.
That is difficult to quantify. I remember people (e.g. Torvalds) complaining about -Wshadow
over a decade ago, which is probably why we have things like -Wshadow=local
or -Wshadow-compatible-local
. While those flags also warn on well defined behavior, I have found them to be more useful in my own experiences.
figure out why the flags set in VSG's project are affecting glslang's project.
Regardless, I agree with @ncesario-lunarg. The impetus for this change feels like the tail wagging the dog.
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.
figure out why the flags set in VSG's project are affecting glslang's project.
That's pretty simple - it uses add_compile_options
to set the warning flags rather than target_compile_options
so the same flags are used for multiple targets automatically, and don't need to be remembered for each one individually. It's not how I'd have set it up, but it's a pretty common approach, and I don't know if I'd be able to talk Robert into changing it.
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.
Basically because until glslang 14, the exported CMake configuration was busted
find_package
support was added recently and is the preferred method going forward, but that is independent of whether or not a repo (VSG in this case) passes down their project-specific flags to a dependent project (glslang in this case), so this is not a rationale for making upstream changes to fix a downstream repo IMO.
This is generally a pretty widespread way of doing things, and lots of people prefer it, even if it's not quite universal and isn't specified in the C++ Core Guidelines.
I do not believe this is true. Based on @jeremy-lunarg's comments, the fact that -Wshadow
is not included in -Wall
, and--as you pointed out--it is not in the C++ core guidelines, the adherence of -Wshadow
is largely subjective and up to the maintainers of glslang (i.e., @arcady-lunarg and @jeremy-lunarg) on whether or not adhering to -Wshadow
's behavior is appropriate for this repo.
That's pretty simple - it uses add_compile_options to set the warning flags rather than target_compile_options so the same flags are used for multiple targets automatically, and don't need to be remembered for each one individually. It's not how I'd have set it up, but it's a pretty common approach, and I don't know if I'd be able to talk Robert into changing it.
Fair enough, but that is not (or shouldn't be IMO) justification for making what I believe is a largely subjective change which doesn't seem to fit here.
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.
Given all these concerns, and the fact that this is ultimately a workaround for a downstream's project deciding to set very conservative warning flags on other projects, I don't think I can accept this PR at this moment.
A revival of #3143, but not on Robert's VulkanSceneGraph-specific branch with extra CMake changes. I've applied the same approach of prefixing names as found in that PR for my extra commits, even though it's not my favourite approach stylistically for avoiding this problem. I don't know if it
Updated for the current state of the codebase so it'll apply cleanly.
I think I found a bug in
SPIRV/GlslangToSpv.cpp
, that would have been prevented had this warning been enabled, and it looks like Robert found an equivalent a year ago, too. It's in a separate commit, and someone should probably check it.I've not actually enabled
-Wshadow
for the project, and that might be something that's wanted.