[RFC] cleanup of frequent shadow warnings #44855
Conversation
…> SystemOfUnits to avoid numerous single character symbols in global scope
|
cms-bot internal usage |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44855/40105
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44855/40106
|
thanks for clarifying; I updated the headers now. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-44855/40112
|
|
Pull request #44855 was updated. @jfernan2, @vlimant, @makortel, @cmsbuild, @rvenditti, @bsunanda, @Dr15Jones, @syuvivida, @mandrenguyen, @smuzaffar, @tjavaid, @fwyzard, @mdhildreth, @subirsarkar, @civanch, @wpmccormack, @valsdav, @srimanob, @antoniovagnerini, @nothingface0, @hqucms can you please check and sign again. |
|
@cmsbuild please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-631d6d/39127/summary.html Comparison SummarySummary:
|
| LaunchParameters(dim3 gridDim, dim3 blockDim, size_t sharedMem = 0, cudaStream_t stream = nullptr) | ||
| : gridDim(gridDim), blockDim(blockDim), sharedMem(sharedMem), stream(stream) {} | ||
| LaunchParameters(dim3 lgridDim, dim3 lblockDim, size_t lsharedMem = 0, cudaStream_t lstream = nullptr) | ||
| : gridDim(lgridDim), blockDim(lblockDim), sharedMem(lsharedMem), stream(lstream) {} | ||
|
|
||
| LaunchParameters(int gridDim, int blockDim, size_t sharedMem = 0, cudaStream_t stream = nullptr) | ||
| : gridDim(gridDim), blockDim(blockDim), sharedMem(sharedMem), stream(stream) {} | ||
| LaunchParameters(int lgridDim, int lblockDim, size_t lsharedMem = 0, cudaStream_t lstream = nullptr) | ||
| : gridDim(lgridDim), blockDim(lblockDim), sharedMem(lsharedMem), stream(lstream) {} |
There was a problem hiding this comment.
Is there any way to tell the compiler "please stop bothering me about this, I know what I am doing and I do like to keep my variable names like they are, thank you very much ?" - other than disabling the warning altogether ?
It's not like a variable can be constructed from itself.
There was a problem hiding this comment.
@fwyzard , I am not an expert but why you need gridDim(gridDim) ? From pedestrian point of view, the name of parameters in a class constructor does not matter, what is matter for a code are names of class members. May be worse to check what gcc experts are thinking and why they introduced this warning. They use to add new warnings, which are not obvious to users, but usually have some arguments (may be very formal).
There was a problem hiding this comment.
The name of any single variable does not matter, the compiled code will happily work in the same way if we call them all aaa, aab, aac, etc.
Of course the code will be very hard to read and follow in that case...
So, yes, we could name the parameters
aunchParameters(dim3 par1, dim3 par2, size_t par3 = 0, cudaStream_t par4 = nullptr)
: gridDim(par1), blockDim(par2), sharedMem(par3), stream(par4) {}Then the code would compile, and -Wshadow would not trigger any exception.
Of course the code would be harder to read and maintain - so we give parameters names that make it easier to read and maintain the code.
If you call the parameter lgridDim - what does the l stand for ?
To improve the naming in that class one could
- rename all member variables from
gridDimtogridDim_; - add accessors for
gridDim_namedgridDim(); - rewrite the constructor to take a parameter named
gridDimand use it to initialise the data membergridDim_.
There was a problem hiding this comment.
well LaunchParameters() : gridDim(gridDim) {} will still compile (with a warning about uninitialized variable)
There was a problem hiding this comment.
If you call the parameter
lgridDim- what does thelstand for ?To improve the naming in that class one could
* rename all member variables from `gridDim` to `gridDim_`; * add accessors for `gridDim_` named `gridDim()`; * rewrite the constructor to take a parameter named `gridDim` and use it to initialise the data member `gridDim_`.
sure, l prefix is somewhat random (I have it to mean local).
@fwyzard
Would gridDim_l (or _a for arg) keep the code still readable in your opinion?
The option to rewrite the class to change members to follow CMSSW naming rules is also possible, but would require more effort.
|
-1 sorry, but for me these changes are simply making the code |
|
@slava77 , may be it is possible to split PR into two: one for "law and order for CLHEP" and another for heterogenous? |
|
"himself" on the subject |
|
Anyhow I'm sure you all read this ttps://stackoverflow.com/questions/2958457/gcc-wshadow-is-too-strict |
|
Anyhow for this trivial example (that is surely not doing what the naive author intended) -Wshadow=local (or compatible-local) will not issue any warning. So: do we want to warn for such cases or we consider the authors able to avoid armful shadow of global (actually class-member) variables? |
yes; this clearly became an RFC PR |
slava77
changed the title
|
@cms-sw/core-l2 please comment if any of these are useful. |
Do you mean in the In |
|
it looks like there is no significant interest to have fixes other than in CLHEP (done in #44882 ) |
While compiling with
-Wshadowfor a test I decided to cleanup some of the more frequent cases exposed mainly by being present in interface/.Perhaps this can be a step towards enabling this warning in our builds.
One curious case was the use of
CLHEP/Units/GlobalSystemOfUnits.h, which puts a few singe character (m, s, g) and other short-name symbols in the global scope. The scopedSystemOfUnits.hseems to be more appropriate for a large sw project.The total count of warnings goes down from 48.6K to 14K.