The checker missed a check for parameter type of primary template of specialization template and this could cause build breakages.
Details
Diff Detail
Event Timeline
Thanks for improving this check!
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp | ||
---|---|---|
148–149 | Could you add a comment here why we're skipping the fix here? | |
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp | ||
388 | Should we apply the fixes or just issue the warning? For virtual methods we suppress the fix since we can't necessarily update all overrides of the method. Are template specializations always guaranteed to be in the same translation unit which would make this safe? |
Thanks for your review!
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp | ||
---|---|---|
148–149 |
Specialization template may match the primary template again by getPreviousDecl. Skipping the fix to avoid repeated fixes for the primary template. | |
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp | ||
388 |
Do you mean that specialization templates are defined in different translation units? If fixing one by one translation unit does have the problem, the readability-const-return-type also has such a problem. clang-tidy can not analyze across translation units, but the diagnosis and fix of it are separate, we can specify the complete compile_commands.json to avoid it. |
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp | ||
---|---|---|
148–149 | Thanks for the explanation. Would you mind adding this as code comment? | |
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp | ||
388 |
I'm not sure how I understand how the compile command json file would help. I think std::hash (https://en.cppreference.com/w/cpp/utility/hash) is an example where you would specialize a type across translation units. If std::hash itself had the issue you wouldn't be able to modify it since it is an external library.
Yeah, this is not an easy balance to strike. ClangTidy is used in many different contexts, in some where you would like to see aggressive fixes, in others where only the safest fixes should be applied. From previous reviews and my own experience we should by default only provide safe fixes with low false positive rate. If you think its' not enough to only notify the user of an inefficiency in template specializations you could add an option to the check for fixing these occurrences. This was done for example here to make a check find issues (including false positives) and report them: https://github.com/llvm/llvm-project/blob/765dd8b8a44cd9689c87c0433739f421b9871061/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp#L26 In summary, I would make this a warning only, but not issue the fix here. |
It looks like precommit CI is failing:
Failed Tests (1):
Clang Tools :: clang-tidy/checkers/performance-unnecessary-value-param.cpp
I'm not quite sure why the test case is not passing on Windows, could you please check this problem? Thanks for your assistance. @flx
I don't have access to Windows, perhaps rebase and try again? It looks like the template specialization related tests are failing there though.
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp | ||
---|---|---|
1 | This will fix your Windows testing issue, I believe. |
LGTM aside from a very minor nit that can be corrected when landing.
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp | ||
---|---|---|
143 | No need for the extra parens. |
clang-tools-extra/docs/ReleaseNotes.rst | ||
---|---|---|
188–191 | This is not at the expected alphabetical place. |
No need for the extra parens.