Sockke added a comment.
Thanks for your review!
================
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,
----------------
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.
================
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();
----------------
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?
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
