flx added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:177-178
const auto &CurrentParam = *FunctionDecl->getParamDecl(Index);
+ if (IsExplicitTemplateSpecialization && Function != FunctionDecl)
+ continue;
Diag << utils::fixit::changeVarDeclToReference(CurrentParam,
----------------
Sockke wrote:
> flx wrote:
> > Could you add a comment here why we're skipping the fix here?
> > Could you add a comment here why we're skipping the fix here?
>
> Specialization template may match the primary template again by
> `getPreviousDecl`. Skipping the fix to avoid repeated fixes for the primary
> template.
Thanks for the explanation. Would you mind adding this as code comment?
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-value-param.cpp:388
+ // CHECK-MESSAGES: [[@LINE-1]]:54: warning: the parameter 'E' is copied
+ // CHECK-FIXES: T templateSpecializationFunction(const ExpensiveToCopyType&
E) {
+ return T();
----------------
Sockke wrote:
> flx wrote:
> > 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?
> > 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?
>
> 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.
> I'm not sure whether this is reasonable, we may make a choice between
> clang-tidy's fault tolerance and advantages. What's your suggestion?
> > 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?
>
> 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.
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.
> I'm not sure whether this is reasonable, we may make a choice between
> clang-tidy's fault tolerance and advantages. What's your suggestion?
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116593/new/
https://reviews.llvm.org/D116593
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
