[clang] [clang-tools-extra] [clang-tidy] Fix performance-unnecessary-value-param (PR #109145)

[Kazu Hirata via cfe-commits]

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 <kazu at 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 <kazu at 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();
   }