https://github.com/kazutakahirata updated https://github.com/llvm/llvm-project/pull/109145
>From c230f7e30e60f11c5675ec1dd9f49f5f6378dc6f Mon Sep 17 00:00:00 2001 From: Kazu Hirata <k...@google.com> Date: Wed, 18 Sep 2024 00:19:25 -0700 Subject: [PATCH 1/2] [clang-tidy] Fix performance-unnecessary-value-param This patch essentially reverts #108674 while adding a testcase that triggers a crash in clang-tidy. Fixes #108963. --- .../unnecessary-value-param-crash.cpp | 23 +++++++++++++++++++ .../Analysis/Analyses/ExprMutationAnalyzer.h | 17 ++++++++++---- 2 files changed, 36 insertions(+), 4 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-crash.cpp diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-crash.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-crash.cpp new file mode 100644 index 00000000000000..99c2fe905bdf37 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-value-param-crash.cpp @@ -0,0 +1,23 @@ +// RUN: %check_clang_tidy -std=c++14-or-later %s performance-unnecessary-value-param %t + +// The test case used to crash clang-tidy. +// https://github.com/llvm/llvm-project/issues/108963 + +struct A +{ + template<typename T> A(T&&) {} +}; + +struct B +{ + ~B(); +}; + +struct C +{ + A a; + C(B, int i) : a(i) {} + // CHECK-MESSAGES: [[@LINE-1]]:6: warning: the parameter #1 is copied for each invocation but only used as a const reference; consider making it a const reference +}; + +C c(B(), 0); diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h index b7b84852168e2e..d7d0d80ee8cd8c 100644 --- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h +++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h @@ -118,10 +118,19 @@ class FunctionParmMutationAnalyzer { static FunctionParmMutationAnalyzer * getFunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context, ExprMutationAnalyzer::Memoized &Memorized) { - auto [it, Inserted] = Memorized.FuncParmAnalyzer.try_emplace(&Func); - if (Inserted) - it->second = std::unique_ptr<FunctionParmMutationAnalyzer>( - new FunctionParmMutationAnalyzer(Func, Context, Memorized)); + auto it = Memorized.FuncParmAnalyzer.find(&Func); + if (it == Memorized.FuncParmAnalyzer.end()) + // We call try_emplace here, repeating a hash lookup on the same key. Note + // that creating a new instance of FunctionParmMutationAnalyzer below may + // add additional elements to FuncParmAnalyzer and invalidate iterators. + // That means that we cannot call try_emplace above and update the value + // portion (i.e. it->second) here. + it = + Memorized.FuncParmAnalyzer + .try_emplace(&Func, std::unique_ptr<FunctionParmMutationAnalyzer>( + new FunctionParmMutationAnalyzer( + Func, Context, Memorized))) + .first; return it->getSecond().get(); } >From ef2522bb8fc645a0db77130a33a0b392cc1fadae Mon Sep 17 00:00:00 2001 From: Kazu Hirata <k...@google.com> Date: Wed, 18 Sep 2024 10:53:46 -0700 Subject: [PATCH 2/2] Add curly braces. Adjust comments. --- .../clang/Analysis/Analyses/ExprMutationAnalyzer.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h index d7d0d80ee8cd8c..c7a5b016c949d0 100644 --- a/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h +++ b/clang/include/clang/Analysis/Analyses/ExprMutationAnalyzer.h @@ -119,18 +119,18 @@ class FunctionParmMutationAnalyzer { getFunctionParmMutationAnalyzer(const FunctionDecl &Func, ASTContext &Context, ExprMutationAnalyzer::Memoized &Memorized) { auto it = Memorized.FuncParmAnalyzer.find(&Func); - if (it == Memorized.FuncParmAnalyzer.end()) - // We call try_emplace here, repeating a hash lookup on the same key. Note - // that creating a new instance of FunctionParmMutationAnalyzer below may - // add additional elements to FuncParmAnalyzer and invalidate iterators. - // That means that we cannot call try_emplace above and update the value - // portion (i.e. it->second) here. + if (it == Memorized.FuncParmAnalyzer.end()) { + // Creating a new instance of FunctionParmMutationAnalyzer below may add + // additional elements to FuncParmAnalyzer. If we did try_emplace before + // creating a new instance, the returned iterator of try_emplace could be + // invalidated. it = Memorized.FuncParmAnalyzer .try_emplace(&Func, std::unique_ptr<FunctionParmMutationAnalyzer>( new FunctionParmMutationAnalyzer( Func, Context, Memorized))) .first; + } return it->getSecond().get(); } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
