from:"Clement Courbet via cfe\-commits"

[clang] [clang analysis][thread safety] Warn when returning rvalue references. (PR #91229)

2024-05-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/91229

We're missing `T&& Consume() && { return std::move(member); }`.

>From 86733474a1bf1486b807c95792781aa2d869c2ca Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 6 May 2024 14:15:47 +
Subject: [PATCH] [clang analysis][thread safety] Warn when returning rvalue
 references too.

We're missing `T&& Consume() && { return std::move(member); }`.
---
 clang/lib/Analysis/ThreadSafety.cpp   |  8 +-
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 25 +++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index e25b843c9bf83e..ce2074a5922e32 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1725,6 +1725,12 @@ void ThreadSafetyAnalyzer::checkAccess(const FactSet 
, const Expr *Exp,
   checkAccess(FSet, ME->getBase(), AK, POK);
   }
 
+  if (const auto *C = dyn_cast(Exp); C && C->isCallToStdMove()) {
+// Changing rvalue-ness of a reference does not change anything w.r.t
+// thread-safety.
+checkAccess(FSet, C->getArg(0), AK, POK);
+  }
+
   const ValueDecl *D = getValueDecl(Exp);
   if (!D || !D->hasAttrs())
 return;
@@ -2160,7 +2166,7 @@ void BuildLockset::VisitReturnStmt(const ReturnStmt *S) {
   // capabilities.
   const QualType ReturnType =
   Analyzer->CurrentFunction->getReturnType().getCanonicalType();
-  if (ReturnType->isLValueReferenceType()) {
+  if (ReturnType->isReferenceType()) {
 Analyzer->checkAccess(
 FunctionExitFSet, RetVal,
 ReturnType->getPointeeType().isConstQualified() ? AK_Read : AK_Written,
diff --git a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp 
b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
index dfb966d3b5902d..03f63227f515cd 100644
--- a/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
+++ b/clang/test/SemaCXX/warn-thread-safety-analysis.cpp
@@ -8,6 +8,26 @@
 
 #include "thread-safety-annotations.h"
 
+
+namespace std {
+
+template  struct remove_reference {
+  using type = T;
+};
+template  struct remove_reference {
+  using type = T;
+};
+template  struct remove_reference {
+  using type = T;
+};
+
+template 
+constexpr typename std::remove_reference::type &(T &) noexcept {
+  return static_cast::type &&>(t);
+}
+
+} // namespace std
+
 class LOCKABLE Mutex {
  public:
   void Lock() EXCLUSIVE_LOCK_FUNCTION();
@@ -5630,6 +5650,11 @@ class Return {
 return foo;   // expected-warning {{returning variable 'foo' 
by reference requires holding mutex 'mu'}}
   }
 
+  Foo &_refref_locked() {
+MutexLock lock();
+return std::move(foo);// expected-warning {{returning variable 'foo' 
by reference requires holding mutex 'mu'}}
+  }
+
   Foo _ref_shared_locks_required() SHARED_LOCKS_REQUIRED(mu) {
 return foo;   // expected-warning {{returning variable 'foo' 
by reference requires holding mutex 'mu' exclusively}}
   }

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] `isOnlyUsedAsConst`: Handle static method calls. (PR #84005)

2024-03-06 Thread Clement Courbet via cfe-commits


legrosbuffle wrote:

> LGTM, verify if some release notes shouldn't be updated.

This is very minor, I've only seen a few instances in the Google codebase, but 
it was trivial enough to include.

https://github.com/llvm/llvm-project/pull/84005
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [llvm] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/82617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [llvm] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-26 Thread Clement Courbet via cfe-commits

legrosbuffle wrote:

Thanks all. Comments addressed.

> Overall looking fine, but this check need to be run on bigger code-base to 
> verify if there are no false positives or crashes.

Sorry, this was not very clear. By "our codebase" I meant the whole of Google 
code :) This also ran on numerous third party repos.

https://github.com/llvm/llvm-project/pull/82617
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [llvm] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/82617

>From 9b93c8bf0614e64352de8e210adf6ff316c0133e Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 19 Feb 2024 14:58:27 +
Subject: [PATCH 1/5] [llvm-exegesis][NFC] Refactor all `ValidationEvent` info
 in a single table.

All data is derived from a single table rather than being spread out
over an enum, a table and the main entry point.
---
 llvm/include/llvm/Target/TargetPfmCounters.td |  1 +
 .../llvm-exegesis/lib/BenchmarkResult.cpp | 49 +--
 .../tools/llvm-exegesis/lib/BenchmarkResult.h | 15 +
 llvm/tools/llvm-exegesis/lib/CMakeLists.txt   |  1 +
 .../lib/LatencyBenchmarkRunner.cpp|  4 +-
 llvm/tools/llvm-exegesis/lib/Target.h |  1 +
 .../llvm-exegesis/lib/ValidationEvent.cpp | 53 
 .../tools/llvm-exegesis/lib/ValidationEvent.h | 60 +++
 llvm/tools/llvm-exegesis/llvm-exegesis.cpp| 20 +--
 9 files changed, 124 insertions(+), 80 deletions(-)
 create mode 100644 llvm/tools/llvm-exegesis/lib/ValidationEvent.cpp
 create mode 100644 llvm/tools/llvm-exegesis/lib/ValidationEvent.h

diff --git a/llvm/include/llvm/Target/TargetPfmCounters.td 
b/llvm/include/llvm/Target/TargetPfmCounters.td
index 8c4d5f50c63a24..cfe432a992b71f 100644
--- a/llvm/include/llvm/Target/TargetPfmCounters.td
+++ b/llvm/include/llvm/Target/TargetPfmCounters.td
@@ -35,6 +35,7 @@ class ValidationEvent  {
   int EventNumber = event_number;
 }
 
+// TableGen names for events defined in `llvm::exegesis::ValidationEvent`.
 def InstructionRetired  : ValidationEvent<0>;
 def L1DCacheLoadMiss : ValidationEvent<1>;
 def L1DCacheStoreMiss : ValidationEvent<2>;
diff --git a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp 
b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
index 189add6464173f..f84ebd2a4e68ef 100644
--- a/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
+++ b/llvm/tools/llvm-exegesis/lib/BenchmarkResult.cpp
@@ -9,6 +9,7 @@
 #include "BenchmarkResult.h"
 #include "BenchmarkRunner.h"
 #include "Error.h"
+#include "ValidationEvent.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
@@ -198,7 +199,7 @@ struct 
CustomMappingTraits> {
   static void inputOne(IO , StringRef KeyStr,
std::map ) {
 Expected Key =
-exegesis::stringToValidationEvent(KeyStr);
+exegesis::getValidationEventByName(KeyStr);
 if (!Key) {
   Io.setError("Key is not a valid validation event");
   return;
@@ -208,7 +209,7 @@ struct 
CustomMappingTraits> {
 
   static void output(IO , std::map ) 
{
 for (auto  : VI) {
-  Io.mapRequired(exegesis::validationEventToString(IndividualVI.first),
+  Io.mapRequired(exegesis::getValidationEventName(IndividualVI.first),
  IndividualVI.second);
 }
   }
@@ -441,49 +442,5 @@ bool operator==(const BenchmarkMeasure , const 
BenchmarkMeasure ) {
  std::tie(B.Key, B.PerInstructionValue, B.PerSnippetValue);
 }
 
-const char *validationEventToString(ValidationEvent VE) {
-  switch (VE) {
-  case exegesis::ValidationEvent::InstructionRetired:
-return "instructions-retired";
-  case exegesis::ValidationEvent::L1DCacheLoadMiss:
-return "l1d-cache-load-misses";
-  case exegesis::ValidationEvent::L1DCacheStoreMiss:
-return "l1d-cache-store-misses";
-  case exegesis::ValidationEvent::L1ICacheLoadMiss:
-return "l1i-cache-load-misses";
-  case exegesis::ValidationEvent::DataTLBLoadMiss:
-return "data-tlb-load-misses";
-  case exegesis::ValidationEvent::DataTLBStoreMiss:
-return "data-tlb-store-misses";
-  case exegesis::ValidationEvent::InstructionTLBLoadMiss:
-return "instruction-tlb-load-misses";
-  case exegesis::ValidationEvent::BranchPredictionMiss:
-return "branch-prediction-misses";
-  }
-  llvm_unreachable("Unhandled exegesis::ValidationEvent enum");
-}
-
-Expected stringToValidationEvent(StringRef Input) {
-  if (Input == "instructions-retired")
-return exegesis::ValidationEvent::InstructionRetired;
-  else if (Input == "l1d-cache-load-misses")
-return exegesis::ValidationEvent::L1DCacheLoadMiss;
-  else if (Input == "l1d-cache-store-misses")
-return exegesis::ValidationEvent::L1DCacheStoreMiss;
-  else if (Input == "l1i-cache-load-misses")
-return exegesis::ValidationEvent::L1ICacheLoadMiss;
-  else if (Input == "data-tlb-load-misses")
-return exegesis::ValidationEvent::DataTLBLoadMiss;
-  else if (Input == "data-tlb-store-misses")
-return exegesis::ValidationEvent::DataTLBStoreMiss;
-  else if (Input == "instruction-tlb-load-misses")
-return exegesis::ValidationEvent::InstructionTLBLoadMiss;
-  else if (Input == "branch-prediction-misses")
-return exegesis::ValidationEvent::BranchPredictionMiss;
-  else
-return make_error("Invalid validation event string",
-   errc::invalid_argument);
-}
-
 } // namespace

[clang-tools-extra] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/82617

>From b2b98e9594b224f471f88924b8b060bee06de483 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Wed, 21 Feb 2024 09:15:22 +
Subject: [PATCH 1/3] [clang-tidy] Add support for determining constness of
 more expressions.

This uses a more systematic approach for determining whcich `DeclRefExpr`s 
mutate
the underlying object: Instead of using a few matchers, we walk up the
AST until we find a parent that we can prove cannot change the
underlying object.

This allows us to handle most address taking and dereference, bindings
to value and const& variables, and track constness of pointee
(see changes in DeclRefExprUtilsTest.cpp).

This allows supporting more patterns in 
`performance-unnecessary-copy-initialization`.

Those two patterns are relatively common:

```
const auto e = (*vector_ptr)[i]
```

and

```
const auto e = vector_ptr->at(i);
```

In our codebase, we have around 25% additional findings from
`performance-unnecessary-copy-initialization` with this change.
I did not see any additional false positives.
---
 .../UnnecessaryCopyInitialization.cpp |  27 +-
 .../clang-tidy/utils/DeclRefExprUtils.cpp | 212 +++---
 .../clang-tidy/utils/DeclRefExprUtils.h   |  33 ++-
 clang-tools-extra/docs/ReleaseNotes.rst   |   6 +
 .../unnecessary-copy-initialization.cpp   |  27 +-
 .../clang-tidy/DeclRefExprUtilsTest.cpp   | 274 +++---
 6 files changed, 396 insertions(+), 183 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index dfe12c5b6007da..9beb185cba929d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
isConstRefReturningMethodCall,
   const auto MethodDecl =
   cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst(
   .bind(MethodDeclId);
-  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverExpr =
+  ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId;
+  const auto OnExpr = anyOf(
+  // Direct reference to `*this`: `a.f()` or `a->f()`.
+  ReceiverExpr,
+  // Access through dereference, typically used for `operator[]`: 
`(*a)[3]`.
+  unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
   const auto ReceiverType =
   hasCanonicalType(recordType(hasDeclaration(namedDecl(
   unless(matchers::matchesAnyListedName(ExcludedContainerTypes));
 
-  return expr(anyOf(
-  cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
-thisPointerType(ReceiverType)),
-  cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
-  hasArgument(0, hasType(ReceiverType);
+  return expr(
+  anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
+  thisPointerType(ReceiverType)),
+cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
+hasArgument(0, hasType(ReceiverType);
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
initializerReturnsReferenceToConst,
 static bool isInitializingVariableImmutable(
 const VarDecl , const Stmt , ASTContext ,
 const std::vector ) {
-  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+  QualType T = InitializingVar.getType().getCanonicalType();
+  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
+ T->isPointerType() ? 1 : 0))
 return false;
 
-  QualType T = InitializingVar.getType().getCanonicalType();
   // The variable is a value type and we know it is only used as const. Safe
   // to reference it and avoid the copy.
   if (!isa(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
   VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
   const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, 
*Result.Context);
   const bool IsVarOnlyUsedAsConst =
-  isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
+  isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context,
+// `NewVar` is always of non-pointer type.
+0);
   const CheckContext Context{
   NewVar,   BlockStmt,   VarDeclStmt, *Result.Context,
   IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp 
b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 2d73179150e8b8..663691c519b8e9 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++

[clang-tools-extra] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/82617

>From b2b98e9594b224f471f88924b8b060bee06de483 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Wed, 21 Feb 2024 09:15:22 +
Subject: [PATCH 1/2] [clang-tidy] Add support for determining constness of
 more expressions.

This uses a more systematic approach for determining whcich `DeclRefExpr`s 
mutate
the underlying object: Instead of using a few matchers, we walk up the
AST until we find a parent that we can prove cannot change the
underlying object.

This allows us to handle most address taking and dereference, bindings
to value and const& variables, and track constness of pointee
(see changes in DeclRefExprUtilsTest.cpp).

This allows supporting more patterns in 
`performance-unnecessary-copy-initialization`.

Those two patterns are relatively common:

```
const auto e = (*vector_ptr)[i]
```

and

```
const auto e = vector_ptr->at(i);
```

In our codebase, we have around 25% additional findings from
`performance-unnecessary-copy-initialization` with this change.
I did not see any additional false positives.
---
 .../UnnecessaryCopyInitialization.cpp |  27 +-
 .../clang-tidy/utils/DeclRefExprUtils.cpp | 212 +++---
 .../clang-tidy/utils/DeclRefExprUtils.h   |  33 ++-
 clang-tools-extra/docs/ReleaseNotes.rst   |   6 +
 .../unnecessary-copy-initialization.cpp   |  27 +-
 .../clang-tidy/DeclRefExprUtilsTest.cpp   | 274 +++---
 6 files changed, 396 insertions(+), 183 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index dfe12c5b6007da..9beb185cba929d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
isConstRefReturningMethodCall,
   const auto MethodDecl =
   cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst(
   .bind(MethodDeclId);
-  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverExpr =
+  ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId;
+  const auto OnExpr = anyOf(
+  // Direct reference to `*this`: `a.f()` or `a->f()`.
+  ReceiverExpr,
+  // Access through dereference, typically used for `operator[]`: 
`(*a)[3]`.
+  unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
   const auto ReceiverType =
   hasCanonicalType(recordType(hasDeclaration(namedDecl(
   unless(matchers::matchesAnyListedName(ExcludedContainerTypes));
 
-  return expr(anyOf(
-  cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
-thisPointerType(ReceiverType)),
-  cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
-  hasArgument(0, hasType(ReceiverType);
+  return expr(
+  anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
+  thisPointerType(ReceiverType)),
+cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
+hasArgument(0, hasType(ReceiverType);
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
initializerReturnsReferenceToConst,
 static bool isInitializingVariableImmutable(
 const VarDecl , const Stmt , ASTContext ,
 const std::vector ) {
-  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+  QualType T = InitializingVar.getType().getCanonicalType();
+  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
+ T->isPointerType() ? 1 : 0))
 return false;
 
-  QualType T = InitializingVar.getType().getCanonicalType();
   // The variable is a value type and we know it is only used as const. Safe
   // to reference it and avoid the copy.
   if (!isa(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
   VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
   const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, 
*Result.Context);
   const bool IsVarOnlyUsedAsConst =
-  isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
+  isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context,
+// `NewVar` is always of non-pointer type.
+0);
   const CheckContext Context{
   NewVar,   BlockStmt,   VarDeclStmt, *Result.Context,
   IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp 
b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 2d73179150e8b8..663691c519b8e9 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++

[clang-tools-extra] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/82617

>From b2b98e9594b224f471f88924b8b060bee06de483 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Wed, 21 Feb 2024 09:15:22 +
Subject: [PATCH] [clang-tidy] Add support for determining constness of more
 expressions.

This uses a more systematic approach for determining whcich `DeclRefExpr`s 
mutate
the underlying object: Instead of using a few matchers, we walk up the
AST until we find a parent that we can prove cannot change the
underlying object.

This allows us to handle most address taking and dereference, bindings
to value and const& variables, and track constness of pointee
(see changes in DeclRefExprUtilsTest.cpp).

This allows supporting more patterns in 
`performance-unnecessary-copy-initialization`.

Those two patterns are relatively common:

```
const auto e = (*vector_ptr)[i]
```

and

```
const auto e = vector_ptr->at(i);
```

In our codebase, we have around 25% additional findings from
`performance-unnecessary-copy-initialization` with this change.
I did not see any additional false positives.
---
 .../UnnecessaryCopyInitialization.cpp |  27 +-
 .../clang-tidy/utils/DeclRefExprUtils.cpp | 212 +++---
 .../clang-tidy/utils/DeclRefExprUtils.h   |  33 ++-
 clang-tools-extra/docs/ReleaseNotes.rst   |   6 +
 .../unnecessary-copy-initialization.cpp   |  27 +-
 .../clang-tidy/DeclRefExprUtilsTest.cpp   | 274 +++---
 6 files changed, 396 insertions(+), 183 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index dfe12c5b6007da..9beb185cba929d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
isConstRefReturningMethodCall,
   const auto MethodDecl =
   cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst(
   .bind(MethodDeclId);
-  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverExpr =
+  ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId;
+  const auto OnExpr = anyOf(
+  // Direct reference to `*this`: `a.f()` or `a->f()`.
+  ReceiverExpr,
+  // Access through dereference, typically used for `operator[]`: 
`(*a)[3]`.
+  unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
   const auto ReceiverType =
   hasCanonicalType(recordType(hasDeclaration(namedDecl(
   unless(matchers::matchesAnyListedName(ExcludedContainerTypes));
 
-  return expr(anyOf(
-  cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
-thisPointerType(ReceiverType)),
-  cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
-  hasArgument(0, hasType(ReceiverType);
+  return expr(
+  anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
+  thisPointerType(ReceiverType)),
+cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
+hasArgument(0, hasType(ReceiverType);
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
initializerReturnsReferenceToConst,
 static bool isInitializingVariableImmutable(
 const VarDecl , const Stmt , ASTContext ,
 const std::vector ) {
-  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+  QualType T = InitializingVar.getType().getCanonicalType();
+  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
+ T->isPointerType() ? 1 : 0))
 return false;
 
-  QualType T = InitializingVar.getType().getCanonicalType();
   // The variable is a value type and we know it is only used as const. Safe
   // to reference it and avoid the copy.
   if (!isa(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
   VarDeclStmt.isSingleDecl() && !NewVar.getLocation().isMacroID();
   const bool IsVarUnused = isVariableUnused(NewVar, BlockStmt, 
*Result.Context);
   const bool IsVarOnlyUsedAsConst =
-  isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context);
+  isOnlyUsedAsConst(NewVar, BlockStmt, *Result.Context,
+// `NewVar` is always of non-pointer type.
+0);
   const CheckContext Context{
   NewVar,   BlockStmt,   VarDeclStmt, *Result.Context,
   IssueFix, IsVarUnused, IsVarOnlyUsedAsConst};
diff --git a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp 
b/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
index 2d73179150e8b8..663691c519b8e9 100644
--- a/clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
+++

[clang-tools-extra] [clang-tidy] Add support for determining constness of more expressions. (PR #82617)

2024-02-22 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/82617

This uses a more systematic approach for determining whcich `DeclRefExpr`s 
mutate the underlying object: Instead of using a few matchers, we walk up the 
AST until we find a parent that we can prove cannot change the underlying 
object.

This allows us to handle most address taking and dereference, bindings to value 
and const& variables, and track constness of pointee (see changes in 
DeclRefExprUtilsTest.cpp).

This allows supporting more patterns in 
`performance-unnecessary-copy-initialization`.

Those two patterns are relatively common:

```
const auto e = (*vector_ptr)[i]
```

and

```
const auto e = vector_ptr->at(i);
```

In our codebase, we have around 25% additional findings from 
`performance-unnecessary-copy-initialization` with this change. I did not see 
any additional false positives.

>From badf08df29e66221c476322bf3a37d3af1ba5eb2 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Wed, 21 Feb 2024 09:15:22 +
Subject: [PATCH] [clang-tidy] Add support for determining constness of more
 expressions.

This uses a more systematic approach for determining whcich `DeclRefExpr`s 
mutate
the underlying object: Instead of using a few matchers, we walk up the
AST until we find a parent that we can prove cannot change the
underlying object.

This allows us to handle most address taking and dereference, bindings
to value and const& variables, and track constness of pointee
(see changes in DeclRefExprUtilsTest.cpp).

This allows supporting more patterns in 
`performance-unnecessary-copy-initialization`.

Those two patterns are relatively common:

```
const auto e = (*vector_ptr)[i]
```

and

```
const auto e = vector_ptr->at(i);
```

In our codebase, we have around 25% additional findings from
`performance-unnecessary-copy-initialization` with this change.
I did not see any additional false positives.
---
 .../UnnecessaryCopyInitialization.cpp |  27 +-
 .../clang-tidy/utils/DeclRefExprUtils.cpp | 212 +++---
 .../clang-tidy/utils/DeclRefExprUtils.h   |  33 ++-
 .../unnecessary-copy-initialization.cpp   |  27 +-
 .../clang-tidy/DeclRefExprUtilsTest.cpp   | 274 +++---
 5 files changed, 390 insertions(+), 183 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index dfe12c5b6007da..9beb185cba929d 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -86,16 +86,22 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
isConstRefReturningMethodCall,
   const auto MethodDecl =
   cxxMethodDecl(returns(hasCanonicalType(matchers::isReferenceToConst(
   .bind(MethodDeclId);
-  const auto ReceiverExpr = declRefExpr(to(varDecl().bind(ObjectArgId)));
+  const auto ReceiverExpr =
+  ignoringParenImpCasts(declRefExpr(to(varDecl().bind(ObjectArgId;
+  const auto OnExpr = anyOf(
+  // Direct reference to `*this`: `a.f()` or `a->f()`.
+  ReceiverExpr,
+  // Access through dereference, typically used for `operator[]`: 
`(*a)[3]`.
+  unaryOperator(hasOperatorName("*"), hasUnaryOperand(ReceiverExpr)));
   const auto ReceiverType =
   hasCanonicalType(recordType(hasDeclaration(namedDecl(
   unless(matchers::matchesAnyListedName(ExcludedContainerTypes));
 
-  return expr(anyOf(
-  cxxMemberCallExpr(callee(MethodDecl), on(ReceiverExpr),
-thisPointerType(ReceiverType)),
-  cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, ReceiverExpr),
-  hasArgument(0, hasType(ReceiverType);
+  return expr(
+  anyOf(cxxMemberCallExpr(callee(MethodDecl), on(OnExpr),
+  thisPointerType(ReceiverType)),
+cxxOperatorCallExpr(callee(MethodDecl), hasArgument(0, OnExpr),
+hasArgument(0, hasType(ReceiverType);
 }
 
 AST_MATCHER_FUNCTION(StatementMatcher, isConstRefReturningFunctionCall) {
@@ -136,10 +142,11 @@ AST_MATCHER_FUNCTION_P(StatementMatcher, 
initializerReturnsReferenceToConst,
 static bool isInitializingVariableImmutable(
 const VarDecl , const Stmt , ASTContext ,
 const std::vector ) {
-  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context))
+  QualType T = InitializingVar.getType().getCanonicalType();
+  if (!isOnlyUsedAsConst(InitializingVar, BlockStmt, Context,
+ T->isPointerType() ? 1 : 0))
 return false;
 
-  QualType T = InitializingVar.getType().getCanonicalType();
   // The variable is a value type and we know it is only used as const. Safe
   // to reference it and avoid the copy.
   if (!isa(T))
@@ -273,7 +280,9 @@ void UnnecessaryCopyInitialization::check(
   VarDeclStmt.isSingleDecl() &&

[clang-tools-extra] [clang] [llvm] [llvm-exegesis] Replace --num-repetitions with --min-instructions (PR #77153)

2024-02-01 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle approved this pull request.


https://github.com/llvm/llvm-project/pull/77153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [llvm] [llvm-exegesis] Replace --num-repetitions with --min-instructions (PR #77153)

2024-02-01 Thread Clement Courbet via cfe-commits


legrosbuffle wrote:

> I'm assuming at some point we'll want to deprecate this old option?

It's a single line of code so it probably can stay there for backwards 
compatibility :)

https://github.com/llvm/llvm-project/pull/77153
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [llvm] [clang-tools-extra] [libc] [llvm-exegesis] Add middle half repetition mode (PR #77020)

2024-01-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle approved this pull request.


https://github.com/llvm/llvm-project/pull/77020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[compiler-rt] [libc] [llvm] [clang-tools-extra] [clang] [libc] `FPRep` builders return `FPRep` instead of raw `StorageType` (PR #78588)

2024-01-22 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle approved this pull request.


https://github.com/llvm/llvm-project/pull/78588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[compiler-rt] [libc] [llvm] [clang-tools-extra] [clang] [libc] `FPRep` builders return `FPRep` instead of raw `StorageType` (PR #78588)

2024-01-22 Thread Clement Courbet via cfe-commits



@@ -535,92 +472,178 @@ struct FPRep : public 
FPRepBase {
 // - Quiet Not a Number
 // - Unnormal
 // This can be reduced to the following logic:
-if (exp_bits() == encode(BiasedExponent::BITS_ALL_ONES()))
+if (exp_bits() == encode(BiasedExp::BITS_ALL_ONES()))
   return !is_inf();
-if (exp_bits() != encode(BiasedExponent::BITS_ALL_ZEROES()))
-  return (sig_bits() & encode(Significand::MSB())) == 0;
+if (exp_bits() != encode(BiasedExp::BITS_ALL_ZEROES()))
+  return (sig_bits() & encode(Sig::MSB())) == 0;
 return false;
   }
   LIBC_INLINE constexpr bool is_quiet_nan() const {
 return exp_sig_bits() >=
-   encode(BiasedExponent::BITS_ALL_ONES(),
-  Significand::MSB() | (Significand::MSB() >> 1));
+   encode(BiasedExp::BITS_ALL_ONES(), Sig::MSB() | (Sig::MSB() >> 1));
   }
   LIBC_INLINE constexpr bool is_signaling_nan() const {
 return is_nan() && !is_quiet_nan();
   }
   LIBC_INLINE constexpr bool is_inf() const {
-return exp_sig_bits() ==
-   encode(BiasedExponent::BITS_ALL_ONES(), Significand::MSB());
-  }
-  LIBC_INLINE constexpr bool is_zero() const {
-return exp_sig_bits() ==
-   encode(BiasedExponent::BITS_ALL_ZEROES(), Significand::ZERO());
+return exp_sig_bits() == encode(BiasedExp::BITS_ALL_ONES(), Sig::MSB());
   }
   LIBC_INLINE constexpr bool is_finite() const {
 return !is_inf() && !is_nan();
   }
   LIBC_INLINE
   constexpr bool is_subnormal() const {
-return exp_sig_bits() >
-   encode(BiasedExponent::BITS_ALL_ZEROES(), Significand::ZERO());
+return exp_bits() == encode(BiasedExp::BITS_ALL_ZEROES());
   }
   LIBC_INLINE constexpr bool is_normal() const {
 const auto exp = exp_bits();
-if (exp == encode(BiasedExponent::BITS_ALL_ZEROES()) ||
-exp == encode(BiasedExponent::BITS_ALL_ONES()))
+if (exp == encode(BiasedExp::BITS_ALL_ZEROES()) ||
+exp == encode(BiasedExp::BITS_ALL_ONES()))
   return false;
 return get_implicit_bit();
   }
+  LIBC_INLINE constexpr StorageType get_explicit_mantissa() const {
+return sig_bits();
+  }
 
-  LIBC_INLINE static constexpr FPRep zero(Sign sign = Sign::POS) {
-return encode(sign, BiasedExponent::BITS_ALL_ZEROES(), 
Significand::ZERO());
+  // This functions is specific to FPRepSem.
+  // TODO: Remove if possible.
+  LIBC_INLINE constexpr bool get_implicit_bit() const {
+return static_cast(bits & EXPLICIT_BIT_MASK);
   }
-  LIBC_INLINE static constexpr FPRep one(Sign sign = Sign::POS) {
-return encode(sign, Exponent::ZERO(), Significand::MSB());
+
+  // This functions is specific to FPRepSem.
+  // TODO: Remove if possible.
+  LIBC_INLINE constexpr void set_implicit_bit(bool implicitVal) {
+if (get_implicit_bit() != implicitVal)
+  bits ^= EXPLICIT_BIT_MASK;
   }
-  LIBC_INLINE static constexpr FPRep min_subnormal(Sign sign = Sign::POS) {
-return encode(sign, BiasedExponent::BITS_ALL_ZEROES(), Significand::LSB());
+};
+
+// 'FPRep' is the bottom of the class hierarchy that only deals with 'FPType'.
+// The operations dealing with specific float semantics are implemented by
+// 'FPRepSem' above and specialized when needed.
+//
+// 'RetT' is the return type used by the builders. If not specified it defaults
+// to the 'StorageType' but 'FPBits' class below defaults it to itself so

legrosbuffle wrote:

Yes, much clearer.

https://github.com/llvm/llvm-project/pull/78588
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)

2024-01-16 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/77105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[lld] [clang] [clang-tools-extra] [llvm] [llvm-exegesis] Add support for validation counters (PR #76653)

2024-01-11 Thread Clement Courbet via cfe-commits



@@ -112,9 +116,11 @@ class Counter {
   PerfEvent Event;
   int FileDescriptor = -1;
   bool IsDummyEvent;
+  std::vector ValidationEvents;

legrosbuffle wrote:

OK, let's rename `Counter` to `CounterGroup`, but let's at least create a 
(private) abstraction for an event and attached FD, to avoid the aligned arrays 
code duplication in `Counter::initRealEvent`:

```
  struct ConfiguredEvent {
llvm::Error init(const pid_t ProcessID, const int GroupFd) {
  constexpr  int Cpu = -1; // measure any processor.
  constexpr int GroupFd = -1; // no grouping of counters.
  constexpr uint32_t Flags = 0;
  perf_event_attr AttrCopy = *Event.attribute();
  FileDescriptor = perf_event_open(, ProcessID, Cpu, GroupFd, 
Flags);
  if (FileDescriptor == -1) {
return ...;
  }
};
PerfEvent Event;
int FileDescriptor = -1;
  };
  // The main event, configured as an ungrouped event.
  ConfiguredEvent MainEvent;
  bool IsDummyEvent;
  // A set of optional validation events grouped into the file descriptor for
  // `MainEvent` using `PERF_IOC_FLAG_GROUP`.
  std::vector ValidationEvents;
```

https://github.com/llvm/llvm-project/pull/76653
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)

2024-01-08 Thread Clement Courbet via cfe-commits



@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
   }
 }
 
+void positiveOnlyAccessedFieldAsConstBinding() {

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/77105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)

2024-01-08 Thread Clement Courbet via cfe-commits



@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
   }
 }
 
+void positiveOnlyAccessedFieldAsConstBinding() {
+  for (auto [X, Y] : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+// CHECK-FIXES: for (const auto& [X, Y] : View>()) {
+use(X);
+use(Y);
+  }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/77105
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)

2024-01-08 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/77105

>From 7420ce9460fa0e07bc570087c39b8ee98775713a Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 5 Jan 2024 15:48:51 +0100
Subject: [PATCH] [clang-tidy] Handle C++ bindings in
 `performance-for-range-copy`

Right now we are not triggering on:

```
for (auto [x, y] : container) {
  // const-only access
}
```
---
 .../performance/ForRangeCopyCheck.cpp | 13 ++--
 clang-tools-extra/docs/ReleaseNotes.rst   |  4 +++
 .../checkers/performance/for-range-copy.cpp   | 31 ---
 clang/lib/Analysis/ExprMutationAnalyzer.cpp   | 13 ++--
 .../Analysis/ExprMutationAnalyzerTest.cpp | 20 
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp 
b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index 5bfa6fb0d02d5c..655e480ffa62cb 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -97,6 +97,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl 
,
   return true;
 }
 
+static bool isReferenced(const VarDecl , const Stmt ,
+ ASTContext ) {
+  const auto IsLoopVar = varDecl(equalsNode());
+  return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf(
+IsLoopVar, bindingDecl(forDecomposition(IsLoopVar,
+Stmt, Context)
+  .empty();
+}
+
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
 const VarDecl , const CXXForRangeStmt ,
 ASTContext ) {
@@ -113,9 +122,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
   // compiler warning which can't be suppressed.
   // Since this case is very rare, it is safe to ignore it.
   if (!ExprMutationAnalyzer(*ForRange.getBody(), Context).isMutated() 
&&
-  !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
- Context)
-   .empty()) {
+  isReferenced(LoopVar, *ForRange.getBody(), Context)) {
 auto Diag = diag(
 LoopVar.getLocation(),
 "loop variable is copied but only used as const reference; consider "
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 4d25e2ebe85f5f..4491d239dc7888 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -419,6 +419,10 @@ Changes in existing checks
   ` check to properly escape
   single quotes.
 
+- Improved :doc:`performance-for-range-copy
+  ` check to handle cases where
+  the loop variable is a structured binding.
+
 - Improved :doc:`performance-noexcept-move-constructor
   ` to better handle
   conditional noexcept expressions, eliminating false-positives.
diff --git 
a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp 
b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
index 1a2eedc9e65c53..f9d06898ca03de 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -47,6 +47,11 @@ struct S {
   S =(const S &);
 };
 
+struct Point {
+  ~Point() {}
+  int x, y;
+};
+
 struct Convertible {
   operator S() const {
 return S();
@@ -87,6 +92,10 @@ void instantiated() {
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is 
{{.*}}
   // CHECK-FIXES: {{^}}  for (const S& S2 : View>()) {}
 
+  for (const auto [X, Y] : View>()) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is
+  // CHECK-FIXES: {{^}}  for (const auto& [X, Y] : View>()) {}
+
   for (const T T2 : View>()) {}
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is 
{{.*}}
   // CHECK-FIXES: {{^}}  for (const T& T2 : View>()) {}
@@ -123,11 +132,6 @@ struct Mutable {
   ~Mutable() {}
 };
 
-struct Point {
-  ~Point() {}
-  int x, y;
-};
-
 Mutable& operator<<(Mutable , bool B) {
   Out.setBool(B);
   return Out;
@@ -144,6 +148,7 @@ void useByValue(Mutable M);
 void useByConstValue(const Mutable M);
 void mutate(Mutable *M);
 void mutate(Mutable );
+void mutate(int &);
 void onceConstOnceMutated(const Mutable , Mutable );
 
 void negativeVariableIsMutated() {
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
   }
 }
 
+void positiveOnlyUsedAsConstBinding() {
+  for (auto [X, Y] : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+// CHECK-FIXES: for (const auto& [X, Y] : View>()) {
+use(X);
+use(Y);
+  }
+}
+
+void negativeMutatedBinding() {
+  for (auto [X, Y] : View>()) {
+use(X);
+mutate(Y);
+  }
+}
+
 void positiveOnlyUsedInCopyConstructor() {
   for (auto A : View>()) {
 // CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but 
only used as

[clang] [clang-tools-extra] [clang-tidy] Handle C++ structured bindings in `performance-for-range-copy` (PR #77105)

2024-01-05 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/77105

Right now we are not triggering on:

```
for (auto [x, y] : container) {
  // const-only access
}
```

>From d5b83c7e8624ba6fde2ea9eb8c3589fe083067b8 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 5 Jan 2024 15:48:51 +0100
Subject: [PATCH] [clang-tidy] Handle C++ bindings in
 `performance-for-range-copy`

Right now we are not triggering on:

```
for (auto [x, y] : container) {
  // const-only access
}
```
---
 .../performance/ForRangeCopyCheck.cpp | 13 ++--
 clang-tools-extra/docs/ReleaseNotes.rst   |  4 +++
 .../checkers/performance/for-range-copy.cpp   | 31 ---
 clang/lib/Analysis/ExprMutationAnalyzer.cpp   | 13 ++--
 .../Analysis/ExprMutationAnalyzerTest.cpp | 20 
 5 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp 
b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
index 5bfa6fb0d02d5c..655e480ffa62cb 100644
--- a/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
@@ -97,6 +97,15 @@ bool ForRangeCopyCheck::handleConstValueCopy(const VarDecl 
,
   return true;
 }
 
+static bool isReferenced(const VarDecl , const Stmt ,
+ ASTContext ) {
+  const auto IsLoopVar = varDecl(equalsNode());
+  return !match(stmt(hasDescendant(declRefExpr(to(valueDecl(anyOf(
+IsLoopVar, bindingDecl(forDecomposition(IsLoopVar,
+Stmt, Context)
+  .empty();
+}
+
 bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
 const VarDecl , const CXXForRangeStmt ,
 ASTContext ) {
@@ -113,9 +122,7 @@ bool ForRangeCopyCheck::handleCopyIsOnlyConstReferenced(
   // compiler warning which can't be suppressed.
   // Since this case is very rare, it is safe to ignore it.
   if (!ExprMutationAnalyzer(*ForRange.getBody(), Context).isMutated() 
&&
-  !utils::decl_ref_expr::allDeclRefExprs(LoopVar, *ForRange.getBody(),
- Context)
-   .empty()) {
+  isReferenced(LoopVar, *ForRange.getBody(), Context)) {
 auto Diag = diag(
 LoopVar.getLocation(),
 "loop variable is copied but only used as const reference; consider "
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 4d25e2ebe85f5f..4491d239dc7888 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -419,6 +419,10 @@ Changes in existing checks
   ` check to properly escape
   single quotes.
 
+- Improved :doc:`performance-for-range-copy
+  ` check to handle cases where
+  the loop variable is a structured binding.
+
 - Improved :doc:`performance-noexcept-move-constructor
   ` to better handle
   conditional noexcept expressions, eliminating false-positives.
diff --git 
a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp 
b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
index 1a2eedc9e65c53..e8c967dacfb7d5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/for-range-copy.cpp
@@ -47,6 +47,11 @@ struct S {
   S =(const S &);
 };
 
+struct Point {
+  ~Point() {}
+  int x, y;
+};
+
 struct Convertible {
   operator S() const {
 return S();
@@ -87,6 +92,10 @@ void instantiated() {
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is 
{{.*}}
   // CHECK-FIXES: {{^}}  for (const S& S2 : View>()) {}
 
+  for (const auto [X, Y] : View>()) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: the loop variable's type is
+  // CHECK-FIXES: {{^}}  for (const auto& [X, Y] : View>()) {}
+
   for (const T T2 : View>()) {}
   // CHECK-MESSAGES: [[@LINE-1]]:16: warning: the loop variable's type is 
{{.*}}
   // CHECK-FIXES: {{^}}  for (const T& T2 : View>()) {}
@@ -123,11 +132,6 @@ struct Mutable {
   ~Mutable() {}
 };
 
-struct Point {
-  ~Point() {}
-  int x, y;
-};
-
 Mutable& operator<<(Mutable , bool B) {
   Out.setBool(B);
   return Out;
@@ -144,6 +148,7 @@ void useByValue(Mutable M);
 void useByConstValue(const Mutable M);
 void mutate(Mutable *M);
 void mutate(Mutable );
+void mutate(int &);
 void onceConstOnceMutated(const Mutable , Mutable );
 
 void negativeVariableIsMutated() {
@@ -234,6 +239,22 @@ void positiveOnlyAccessedFieldAsConst() {
   }
 }
 
+void positiveOnlyAccessedFieldAsConstBinding() {
+  for (auto [X, Y] : View>()) {
+// CHECK-MESSAGES: [[@LINE-1]]:13: warning: loop variable is copied but
+// CHECK-FIXES: for (const auto& [X, Y] : View>()) {
+use(X);
+use(Y);
+  }
+}
+
+void negativeOnlyAccessedFieldAsConstBinding() {
+  for (auto [X, Y] : View>()) {
+use(X);
+mutate(Y);
+  }
+}
+
 void

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits


legrosbuffle wrote:

Thanks

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits



@@ -263,19 +264,26 @@ void 
UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
 
 void UnnecessaryCopyInitialization::check(
 const MatchFinder::MatchResult ) {
-  const auto *NewVar = Result.Nodes.getNodeAs("newVarDecl");
+  const auto  = *Result.Nodes.getNodeAs("newVarDecl");
+  const auto  = *Result.Nodes.getNodeAs("blockStmt");
+  const auto  = *Result.Nodes.getNodeAs("declStmt");
+  const CheckContext Context{
+  NewVar, BlockStmt, VarDeclStmt, *Result.Context,
+  // Do not propose fixes if the DeclStmt has multiple VarDecls or in
+  // macros since we cannot place them correctly.
+  /*IssueFix*/ VarDeclStmt.isSingleDecl() &&

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits



@@ -289,74 +297,72 @@ void UnnecessaryCopyInitialization::check(
   // instantiations where the types differ and rely on implicit conversion 
would
   // no longer compile if we switched to a reference.
   if (differentReplacedTemplateParams(
-  NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes),
+  Context.Var.getType(), constructorArgumentType(OldVar, Result.Nodes),
   *Result.Context))
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+// `auto NewVar = functionCall();`
+handleCopyFromMethodReturn(Context, ObjectArg);
   } else {
-handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-   *Result.Context);
+// `auto NewVar = OldVar;`
+handleCopyFromLocalVar(Context, *OldVar);
   }
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
-const VarDecl , const Stmt , const DeclStmt ,
-bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
-  bool IsConstQualified = Var.getType().isConstQualified();
-  if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
+const CheckContext , const VarDecl *ObjectArg) {
+  bool IsConstQualified = Ctx.Var.getType().isConstQualified();
+  if (!IsConstQualified && !Ctx.IsVarOnlyUsedAsConst)
 return;
   if (ObjectArg != nullptr &&
-  !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
+  !isInitializingVariableImmutable(*ObjectArg, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+  diagnoseCopyFromMethodReturn(Ctx);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
-const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
-  !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
+const CheckContext , const VarDecl ) {
+  if (!Ctx.IsVarOnlyUsedAsConst ||
+  !isInitializingVariableImmutable(OldVar, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
 return;
+  diagnoseCopyFromLocalVar(Ctx, OldVar);
+}
 
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
+void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
+const CheckContext ) {
+  auto Diagnostic =
+  diag(Ctx.Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is "
+   "copy-constructed "
+   "from a const reference%select{%select{ but is only used as const "
+   "reference|}0| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << Ctx.Var.getType().isConstQualified() <<  << Ctx.IsVarUnused;
+  maybeIssueFixes(Ctx, Diagnostic);
+}
+
+void UnnecessaryCopyInitialization::diagnoseCopyFromLocalVar(
+const CheckContext , const VarDecl ) {
+  auto Diagnostic =
+  diag(Ctx.Var.getLocation(),
+   "local copy %1 of the variable %0 is never modified%select{"
+   "| and never used}2; consider %select{avoiding the copy|removing "
+   "the statement}2")
+  <<  <<  << Ctx.IsVarUnused;
+  maybeIssueFixes(Ctx, Diagnostic);
+}
+
+void UnnecessaryCopyInitialization::maybeIssueFixes(
+const CheckContext , DiagnosticBuilder ) {
+  if (Ctx.IssueFix) {
+if (Ctx.IsVarUnused) {

legrosbuffle wrote:

Done.

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH 1/5] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 73 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfc..a9ef3faf8c343 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (!IssueFix)
+return;
+  if (IsVarUnused)
+recordRemoval(Stmt, Context, Diagnostic);
+  else
+recordFixes(Var, Context, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +326,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits



@@ -261,21 +262,27 @@ void 
UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
  this);
 }
 
+UnnecessaryCopyInitialization::CheckContext::CheckContext(

legrosbuffle wrote:

I think it's a simple matter of preference, I think we can see the constructor 
as a factory function for the context.

To me the following are all kind of equivalent:
  - `CheckContext Context(Result);`
  - `CheckContext Context = MakeContext(Result);`
  - `CheckContext Context{NewVar, ...}`

With the caveat that the latter allows putting the context in an invalid state 
(e.g. with `IsVarUnused` in an inconsistent value w.r.t. `Var`). Anyway I don't 
feel strongly, so I've switched to a raw struct without ctor as suggested. 


https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH 1/4] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 73 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfc..a9ef3faf8c343 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (!IssueFix)
+return;
+  if (IsVarUnused)
+recordRemoval(Stmt, Context, Diagnostic);
+  else
+recordFixes(Var, Context, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +326,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH 1/3] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 73 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfc..a9ef3faf8c343 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (!IssueFix)
+return;
+  if (IsVarUnused)
+recordRemoval(Stmt, Context, Diagnostic);
+  else
+recordFixes(Var, Context, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +326,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle edited 
https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits



@@ -32,14 +32,34 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck 
{
   void check(const ast_matchers::MatchFinder::MatchResult ) override;
   void storeOptions(ClangTidyOptions::OptionMap ) override;
 
+protected:
+  // A helper to manipulate the state common to
+  // `CopyFromMethodReturn` and `CopyFromLocalVar`.
+  struct CheckContext {
+CheckContext(const ast_matchers::MatchFinder::MatchResult );
+const VarDecl 
+const Stmt 
+const DeclStmt 
+clang::ASTContext 
+const bool IssueFix;
+const bool IsVarUnused;
+const bool IsVarOnlyUsedAsConst;
+  };
+
+  // Create diagnostics. These are virtual so that derived classes can change
+  // behaviour.
+  virtual void diagnoseCopyFromMethodReturn(const CheckContext ,

legrosbuffle wrote:

`ObjectArg` is a pointer because it can be null (unlike other variables, which 
are always non-null. Anyway, this is no longer provided as pre the previous 
comment.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits



@@ -290,69 +296,72 @@ void UnnecessaryCopyInitialization::check(
   // instantiations where the types differ and rely on implicit conversion 
would
   // no longer compile if we switched to a reference.
   if (differentReplacedTemplateParams(
-  NewVar->getType(), constructorArgumentType(OldVar, Result.Nodes),
+  Context.Var.getType(), constructorArgumentType(OldVar, Result.Nodes),
   *Result.Context))
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+// `auto NewVar = functionCall();`
+handleCopyFromMethodReturn(Context, ObjectArg);
   } else {
-handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-   *Result.Context);
+// `auto NewVar = OldVar;`
+handleCopyFromLocalVar(Context, *OldVar);
   }
 }
 
-void UnnecessaryCopyInitialization::makeDiagnostic(
-DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
-const DeclStmt , ASTContext , bool IssueFix) {
-  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
-  Diagnostic <<  << IsVarUnused;
-  if (!IssueFix)
-return;
-  if (IsVarUnused)
-recordRemoval(Stmt, Context, Diagnostic);
-  else
-recordFixes(Var, Context, Diagnostic);
-}
-
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
-const VarDecl , const Stmt , const DeclStmt ,
-bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
-  bool IsConstQualified = Var.getType().isConstQualified();
-  if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
+const CheckContext , const VarDecl *ObjectArg) {
+  bool IsConstQualified = Ctx.Var.getType().isConstQualified();
+  if (!IsConstQualified && !Ctx.IsVarOnlyUsedAsConst)
 return;
   if (ObjectArg != nullptr &&
-  !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
+  !isInitializingVariableImmutable(*ObjectArg, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
 return;
-
-  auto Diagnostic =
-  diag(Var.getLocation(),
-   "the %select{|const qualified }0variable %1 is copy-constructed "
-   "from a const reference%select{"
-   "%select{ but is only used as const reference|}0"
-   "| but is never used}2; consider "
-   "%select{making it a const reference|removing the statement}2")
-  << IsConstQualified;
-  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
- IssueFix);
+  diagnoseCopyFromMethodReturn(Ctx, ObjectArg);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
-const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
-  !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
+const CheckContext , const VarDecl ) {
+  if (!Ctx.IsVarOnlyUsedAsConst ||
+  !isInitializingVariableImmutable(OldVar, Ctx.BlockStmt, Ctx.ASTCtx,
ExcludedContainerTypes))
 return;
-  auto Diagnostic = diag(Var.getLocation(),
- "local copy %1 of the variable %0 is never modified"
- "%select{| and never used}2; consider "
- "%select{avoiding the copy|removing the statement}2")
-<< 
-  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
- IssueFix);
+  diagnoseCopyFromLocalVar(Ctx, OldVar);
+}
+
+void UnnecessaryCopyInitialization::diagnoseCopyFromMethodReturn(
+const CheckContext , const VarDecl *ObjectArg) {

legrosbuffle wrote:

Thanks for the catch, this is a leftover from the previous iteration. Removed. 

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-06 Thread Clement Courbet via cfe-commits



@@ -261,21 +262,27 @@ void 
UnnecessaryCopyInitialization::registerMatchers(MatchFinder *Finder) {
  this);
 }
 
+UnnecessaryCopyInitialization::CheckContext::CheckContext(

legrosbuffle wrote:

Given that we are deriving `IssueFix`, `IsVarUnused`, `IsVarOnlyUsedAsConst` 
from `Var`, `BlockStmt`, ... it sounds natural to have a ctor there. Otherwise 
we risk creating a `CheckContext` in an invalid state.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-01 Thread Clement Courbet via cfe-commits



@@ -302,6 +303,20 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,

legrosbuffle wrote:

Done. I've kept the code common to these two functions factored out into 
`maybeIssueFixes`, and I've created a helper struct for the 5 common parameters 
to these two functions (and the additional 2 boolean variables that these two 
have in common).

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-01 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH 1/2] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 73 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..a9ef3faf8c343c9 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (!IssueFix)
+return;
+  if (IsVarUnused)
+recordRemoval(Stmt, Context, Diagnostic);
+  else
+recordFixes(Var, Context, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +326,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-12-01 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH 1/2] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 73 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..a9ef3faf8c343c9 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (!IssueFix)
+return;
+  if (IsVarUnused)
+recordRemoval(Stmt, Context, Diagnostic);
+  else
+recordFixes(Var, Context, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +326,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 851460af6526f175bc34b105a0f5f130a2f1c6b1 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 73 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 41 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..a9ef3faf8c343c9 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,19 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (!IssueFix)
+return;
+  if (IsVarUnused)
+recordRemoval(Stmt, Context, Diagnostic);
+  else
+recordFixes(Var, Context, Diagnostic);
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +326,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits



@@ -302,6 +303,20 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (IssueFix) {
+if (IsVarUnused) {

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits



@@ -302,6 +303,20 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (IssueFix) {

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits



@@ -302,6 +303,20 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,

legrosbuffle wrote:

I can't really do that because each of the two uses of this function are 
streaming one description-dependent parameter (`IsConstQualified` for one and 
`OldVar` for the other). These are even different types. So it feels simpler to 
stream before entering the function, so that the function only holds logic 
which is common to both callers.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


legrosbuffle wrote:

This is ready for another look. I took this as an opportunity to refactor the 
duplicated parts of the code. Let me know what you think.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 0bf4d8335063c1403dc91d8fccca42e3f03bad35 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 74 +--
 .../UnnecessaryCopyInitialization.h   |  7 ++
 2 files changed, 42 insertions(+), 39 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..b2775ac021f4505 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -302,6 +303,20 @@ void UnnecessaryCopyInitialization::check(
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (IssueFix) {
+if (IsVarUnused) {
+  recordRemoval(Stmt, Context, Diagnostic);
+} else {
+  recordFixes(Var, Context, Diagnostic);
+}
+  }
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +327,33 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic = diag(Var.getLocation(),
+ "local copy %1 of the variable %0 is never modified"
+ "%select{| and never used}2; consider "
+ "%select{avoiding the copy|removing the statement}2")
+<< 
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From a072ae129b1ad928c96368a3208d35cc0705e260 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init

Refactor diagnostic emission and add a hook so that derived checks
can observe for which variables a warning has been emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 80 +--
 .../UnnecessaryCopyInitialization.h   |  6 ++
 2 files changed, 45 insertions(+), 41 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..5b45e911fd11ed0 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -294,14 +295,28 @@ void UnnecessaryCopyInitialization::check(
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+   ObjectArg, *Result.Context);
   } else {
 handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
*Result.Context);
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (IssueFix) {
+if (IsVarUnused) {
+  recordRemoval(Stmt, Context, Diagnostic);
+} else {
+  recordFixes(Var, Context, Diagnostic);
+}
+  }
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +327,34 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic =
+  diag(Var.getLocation(),
+

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From d729f95270aff076ff11ccb1ef12cfb6dc99a82d Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init: Add a hook...

... so that derived checks can observe for which variables a warning has been 
emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 80 +--
 .../UnnecessaryCopyInitialization.h   |  6 ++
 2 files changed, 45 insertions(+), 41 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..5b45e911fd11ed0 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -15,6 +15,7 @@
 #include "clang/AST/Decl.h"
 #include "clang/Basic/Diagnostic.h"
 #include 
+#include 
 
 namespace clang::tidy::performance {
 namespace {
@@ -294,14 +295,28 @@ void UnnecessaryCopyInitialization::check(
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+   ObjectArg, *Result.Context);
   } else {
 handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
*Result.Context);
   }
 }
 
+void UnnecessaryCopyInitialization::makeDiagnostic(
+DiagnosticBuilder Diagnostic, const VarDecl , const Stmt ,
+const DeclStmt , ASTContext , bool IssueFix) {
+  const bool IsVarUnused = isVariableUnused(Var, BlockStmt, Context);
+  Diagnostic <<  << IsVarUnused;
+  if (IssueFix) {
+if (IsVarUnused) {
+  recordRemoval(Stmt, Context, Diagnostic);
+} else {
+  recordFixes(Var, Context, Diagnostic);
+}
+  }
+}
+
 void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
@@ -312,52 +327,34 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-  if (isVariableUnused(Var, BlockStmt, Context)) {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference but is never used; consider "
- "removing the statement")
-<< IsConstQualified << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(Var.getLocation(),
- "the %select{|const qualified }0variable %1 is copy-constructed "
- "from a const reference%select{ but is only used as const "
- "reference|}0; consider making it a const reference")
-<< IsConstQualified << 
-if (IssueFix)
-  recordFixes(Var, Context, Diagnostic);
-  }
+
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "the %select{|const qualified }0variable %1 is copy-constructed "
+   "from a const reference%select{"
+   "%select{ but is only used as const reference|}0"
+   "| but is never used}2; consider "
+   "%select{making it a const reference|removing the statement}2")
+  << IsConstQualified;
+  makeDiagnostic(std::move(Diagnostic), Var, BlockStmt, Stmt, Context,
+ IssueFix);
 }
 
 void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
-const VarDecl , const VarDecl , const Stmt ,
+const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
-  if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
+  if (!isOnlyUsedAsConst(Var, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
 return;
-
-  if (isVariableUnused(NewVar, BlockStmt, Context)) {
-auto Diagnostic = diag(NewVar.getLocation(),
-   "local copy %0 of the variable %1 is never modified 
"
-   "and never used; "
-   "consider removing the statement")
-  <<  << 
-if (IssueFix)
-  recordRemoval(Stmt, Context, Diagnostic);
-  } else {
-auto Diagnostic =
-diag(NewVar.getLocation(),
- "local copy %0 of the variable %1 is never modified; "
- "consider avoiding the copy")
-<<  << 
-if (IssueFix)
-  recordFixes(NewVar, Context, Diagnostic);
-  }
+  auto Diagnostic =
+  diag(Var.getLocation(),
+   "local copy %1 of the

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


legrosbuffle wrote:

> In theory changes that are required for "private" checks should be done by 
> you on some own private fork instead of adding that "unused" code here. But 
> only option I see is simply moving diagnostic to separate methods, and make 
> those methods virtual. 

I see, we don't want code that's dead upstream. Makes sense. I'll do something 
like what you're suggesting.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From a6bc3d7ef798f95fe6ae758e7a9502851e6d4b12 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init: Add a hook...

... so that derived checks can observe for which variables a warning has been 
emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 24 ---
 .../UnnecessaryCopyInitialization.h   |  9 +--
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..1ba32315afe0d35 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -294,24 +294,28 @@ void UnnecessaryCopyInitialization::check(
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+if (handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+   ObjectArg, *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   } else {
-handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-   *Result.Context);
+if (handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
+   *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   }
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
+bool UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
-return;
+return false;
   if (ObjectArg != nullptr &&
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
   if (isVariableUnused(Var, BlockStmt, Context)) {
 auto Diagnostic =
 diag(Var.getLocation(),
@@ -331,15 +335,16 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 if (IssueFix)
   recordFixes(Var, Context, Diagnostic);
   }
+  return true;
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
+bool UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
 
   if (isVariableUnused(NewVar, BlockStmt, Context)) {
 auto Diagnostic = diag(NewVar.getLocation(),
@@ -358,6 +363,7 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 if (IssueFix)
   recordFixes(NewVar, Context, Diagnostic);
   }
+  return true;
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index ea009ba9979de97..5310c29fad3672c 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,13 +33,18 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck 
{
   void storeOptions(ClangTidyOptions::OptionMap ) override;
 
 private:
-  void handleCopyFromMethodReturn(const VarDecl , const Stmt ,
+  bool handleCopyFromMethodReturn(const VarDecl , const Stmt ,
   const DeclStmt , bool IssueFix,
   const VarDecl *ObjectArg,
   ASTContext );
-  void handleCopyFromLocalVar(const VarDecl , const VarDecl ,
+  bool handleCopyFromLocalVar(const VarDecl , const VarDecl ,
   const Stmt , const DeclStmt ,
   bool IssueFix, ASTContext );
+
+  // A hook called after each warnings is emitted.
+  virtual void onWarningEmitted(const VarDecl , const Stmt ,
+ASTContext ) {}
+
   const std::vector AllowedTypes;
   const std::vector ExcludedContainerTypes;
 };

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From 6050ba4784137bc20e58d553ea970e37237142ae Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init: Add a hook...

... so that derived checks can observe for which variables a warning has been 
emitted.
---
 .../UnnecessaryCopyInitialization.cpp | 24 ---
 .../UnnecessaryCopyInitialization.h   |  9 +--
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..1ba32315afe0d35 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -294,24 +294,28 @@ void UnnecessaryCopyInitialization::check(
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+if (handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+   ObjectArg, *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   } else {
-handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-   *Result.Context);
+if (handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
+   *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   }
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
+bool UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
-return;
+return false;
   if (ObjectArg != nullptr &&
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
   if (isVariableUnused(Var, BlockStmt, Context)) {
 auto Diagnostic =
 diag(Var.getLocation(),
@@ -331,15 +335,16 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 if (IssueFix)
   recordFixes(Var, Context, Diagnostic);
   }
+  return true;
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
+bool UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
 
   if (isVariableUnused(NewVar, BlockStmt, Context)) {
 auto Diagnostic = diag(NewVar.getLocation(),
@@ -358,6 +363,7 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 if (IssueFix)
   recordFixes(NewVar, Context, Diagnostic);
   }
+  return true;
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index ea009ba9979de97..7d76c9e7cab940c 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,13 +33,18 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck 
{
   void storeOptions(ClangTidyOptions::OptionMap ) override;
 
 private:
-  void handleCopyFromMethodReturn(const VarDecl , const Stmt ,
+  bool handleCopyFromMethodReturn(const VarDecl , const Stmt ,
   const DeclStmt , bool IssueFix,
   const VarDecl *ObjectArg,
   ASTContext );
-  void handleCopyFromLocalVar(const VarDecl , const VarDecl ,
+  bool handleCopyFromLocalVar(const VarDecl , const VarDecl ,
   const Stmt , const DeclStmt ,
   bool IssueFix, ASTContext );
+
+  // A hook called for each emitted warning.
+  virtual void onWarningEmitted(const VarDecl , const Stmt ,
+ASTContext ) {}
+
   const std::vector AllowedTypes;
   const std::vector ExcludedContainerTypes;
 };

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle edited 
https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits

legrosbuffle wrote:

> First what's a purpose of this change. Is there any other check that will 
> need it or not ?

We have a downstream check that wants to do additional processing after this 
one. We can't submit that check because it depends on a large piece of 
downstream infrastructure.

> With current implementation I don't see any "custom behaviour" that could be 
> implemented, as this act more like observer.

Indeed, the downstream check does not change the behaviour, it does extra stuff 
in addition what the check already does. I've rephrased the commit message to 
make that clearer.

https://github.com/llvm/llvm-project/pull/73921
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/73921

>From e0d1f9741d0dba3286fd8043cf6d5c89ecc2d378 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init: Add a hook...

... so that derived checks can implement custom behaviour.
---
 .../UnnecessaryCopyInitialization.cpp | 24 ---
 .../UnnecessaryCopyInitialization.h   |  9 +--
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..1ba32315afe0d35 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -294,24 +294,28 @@ void UnnecessaryCopyInitialization::check(
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+if (handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+   ObjectArg, *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   } else {
-handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-   *Result.Context);
+if (handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
+   *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   }
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
+bool UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
-return;
+return false;
   if (ObjectArg != nullptr &&
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
   if (isVariableUnused(Var, BlockStmt, Context)) {
 auto Diagnostic =
 diag(Var.getLocation(),
@@ -331,15 +335,16 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 if (IssueFix)
   recordFixes(Var, Context, Diagnostic);
   }
+  return true;
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
+bool UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
 
   if (isVariableUnused(NewVar, BlockStmt, Context)) {
 auto Diagnostic = diag(NewVar.getLocation(),
@@ -358,6 +363,7 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 if (IssueFix)
   recordFixes(NewVar, Context, Diagnostic);
   }
+  return true;
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index ea009ba9979de97..7d76c9e7cab940c 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,13 +33,18 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck 
{
   void storeOptions(ClangTidyOptions::OptionMap ) override;
 
 private:
-  void handleCopyFromMethodReturn(const VarDecl , const Stmt ,
+  bool handleCopyFromMethodReturn(const VarDecl , const Stmt ,
   const DeclStmt , bool IssueFix,
   const VarDecl *ObjectArg,
   ASTContext );
-  void handleCopyFromLocalVar(const VarDecl , const VarDecl ,
+  bool handleCopyFromLocalVar(const VarDecl , const VarDecl ,
   const Stmt , const DeclStmt ,
   bool IssueFix, ASTContext );
+
+  // A hook called for each emitted warning.
+  virtual void onWarningEmitted(const VarDecl , const Stmt ,
+ASTContext ) {}
+
   const std::vector AllowedTypes;
   const std::vector ExcludedContainerTypes;
 };

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [clang-tidy] performance-unnecessary-copy-init: Add a hook... (PR #73921)

2023-11-30 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/73921

... so that derived checks can implement custom behaviour. Does nothing by 
default, which makes this  an NFC.

>From e251c440b42fe67dee4022d08d05e96d30d6aa02 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Thu, 30 Nov 2023 11:08:51 +0100
Subject: [PATCH] [clang-tidy] performance-unnecessary-copy-init: Add a hook...

... so that derived checks can implement custom behaviour.
---
 .../UnnecessaryCopyInitialization.cpp | 24 ---
 .../UnnecessaryCopyInitialization.h   |  9 +--
 2 files changed, 22 insertions(+), 11 deletions(-)

diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
index 990e20400fbfcd2..966071e92833b87 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
@@ -294,24 +294,28 @@ void UnnecessaryCopyInitialization::check(
 return;
 
   if (OldVar == nullptr) {
-handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix, ObjectArg,
-   *Result.Context);
+if (handleCopyFromMethodReturn(*NewVar, *BlockStmt, *Stmt, IssueFix,
+   ObjectArg, *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}
   } else {
-handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
-   *Result.Context);
+if (handleCopyFromLocalVar(*NewVar, *OldVar, *BlockStmt, *Stmt, IssueFix,
+   *Result.Context)) {
+  onWarningEmitted(*NewVar, *BlockStmt, *Result.Context);
+}  
   }
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
+bool UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 const VarDecl , const Stmt , const DeclStmt ,
 bool IssueFix, const VarDecl *ObjectArg, ASTContext ) {
   bool IsConstQualified = Var.getType().isConstQualified();
   if (!IsConstQualified && !isOnlyUsedAsConst(Var, BlockStmt, Context))
-return;
+return false;
   if (ObjectArg != nullptr &&
   !isInitializingVariableImmutable(*ObjectArg, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
   if (isVariableUnused(Var, BlockStmt, Context)) {
 auto Diagnostic =
 diag(Var.getLocation(),
@@ -331,15 +335,16 @@ void 
UnnecessaryCopyInitialization::handleCopyFromMethodReturn(
 if (IssueFix)
   recordFixes(Var, Context, Diagnostic);
   }
+  return true;
 }
 
-void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
+bool UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 const VarDecl , const VarDecl , const Stmt ,
 const DeclStmt , bool IssueFix, ASTContext ) {
   if (!isOnlyUsedAsConst(NewVar, BlockStmt, Context) ||
   !isInitializingVariableImmutable(OldVar, BlockStmt, Context,
ExcludedContainerTypes))
-return;
+return false;
 
   if (isVariableUnused(NewVar, BlockStmt, Context)) {
 auto Diagnostic = diag(NewVar.getLocation(),
@@ -358,6 +363,7 @@ void UnnecessaryCopyInitialization::handleCopyFromLocalVar(
 if (IssueFix)
   recordFixes(NewVar, Context, Diagnostic);
   }
+  return true;
 }
 
 void UnnecessaryCopyInitialization::storeOptions(
diff --git 
a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h 
b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
index ea009ba9979de97..7d76c9e7cab940c 100644
--- a/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
+++ b/clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.h
@@ -33,13 +33,18 @@ class UnnecessaryCopyInitialization : public ClangTidyCheck 
{
   void storeOptions(ClangTidyOptions::OptionMap ) override;
 
 private:
-  void handleCopyFromMethodReturn(const VarDecl , const Stmt ,
+  bool handleCopyFromMethodReturn(const VarDecl , const Stmt ,
   const DeclStmt , bool IssueFix,
   const VarDecl *ObjectArg,
   ASTContext );
-  void handleCopyFromLocalVar(const VarDecl , const VarDecl ,
+  bool handleCopyFromLocalVar(const VarDecl , const VarDecl ,
   const Stmt , const DeclStmt ,
   bool IssueFix, ASTContext );
+
+  // A hook called for each emitted warning.
+  virtual void onWarningEmitted(const VarDecl , const Stmt ,
+ASTContext ) {}
+
   const std::vector AllowedTypes;
   const std::vector ExcludedContainerTypes;
 };

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/68572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits



@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/68572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From f3b0c9708bbf6fb0962fb82b605a7fc0b52d4a5b Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`, which is on by default under 
`-Wthread-safety-reference`.

 - People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td |  4 +-
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 78 --
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 +++
 6 files changed, 163 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 674eb9f4ef2e73f..2e4e22e4f90bee8 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1066,7 +1066,9 @@ def Most : DiagGroup<"most", [
 def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
 def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
 def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference  : DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
 def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 7f39f5e79792c07..fb281773fdbf819 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..7fdf22c2f3919cb 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const 
CapabilityExpr ) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
-if (!CurrentMethod)
+if (!isa_and_nonnull(CurrentFunction))
   return false;
 const ValueDecl *VD =

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-19 Thread Clement Courbet via cfe-commits



@@ -2278,7 +2303,7 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
   PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
 
   CFGBlockInfo  = BlockInfo[CFGraph->getEntry().getBlockID()];
-  CFGBlockInfo= BlockInfo[CFGraph->getExit().getBlockID()];
+  CFGBlockInfo  = BlockInfo[CFGraph->getExit().getBlockID()];

legrosbuffle wrote:

Done.

https://github.com/llvm/llvm-project/pull/68572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle edited 
https://github.com/llvm/llvm-project/pull/68572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From db868042066e8b985d20c1b728729ba7102aaefa Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`, which is on by default under 
`-Wthread-safety-reference`.

 - People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 12 +--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..a71afdd710c3bf4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 58a33e2807b7b0a..4c456a5561dc265 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits



@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;

legrosbuffle wrote:

Done, thanks.

https://github.com/llvm/llvm-project/pull/68572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From 2d90dc0547f90c3a217428b2d3b8d2253ae80973 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 12 +--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..a71afdd710c3bf4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 58a33e2807b7b0a..4c456a5561dc265 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-17 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/68572

>From 11f5286f426d082f7fbcb578c0c6cabcd3660453 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 12 +--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 168 insertions(+), 33 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..a71afdd710c3bf4 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,11 +1061,13 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference",
+ [ThreadSafetyReferenceReturn]>;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b211680a0e9b6e9..c777d73dd4a769b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-09 Thread Clement Courbet via cfe-commits


legrosbuffle wrote:

@aeubanks FYI

https://github.com/llvm/llvm-project/pull/68572
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68572)

2023-10-09 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/68572

…… (#68394)"

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.

>From a801efe3a6799df6ecee7ddf1ce77572d756cabb Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Mon, 9 Oct 2023 10:20:12 +0200
Subject: [PATCH] =?UTF-8?q?Reapply=20"[clang=20analysis][thread-safety]=20?=
 =?UTF-8?q?Handle=20return-by-reference..=E2=80=A6=20(#68394)"?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The new warnings are now under a separate flag 
`-Wthread-safety-reference-return`. The plan is:

- Start with a period where people can opt into the new warnings with 
`-Wthread-safety-reference-return` or `-Wthread-safety-beta`. This allows 
downstream to test the new warnings without having to roll the implementation 
back and forth.
- Make `-Wthread-safety-reference-return` part of `-Wthread-safety-reference`. 
People can opt out via `-Wthread-safety-reference 
-Wnothread-safety-reference-return`.
- (maybe) delete `-Wthread-safety-reference-return` after some time ?

This reverts commit 859f2d032386632562521a99db20923217d98988.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 clang/include/clang/Basic/DiagnosticGroups.td | 14 ++--
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 6 files changed, 169 insertions(+), 34 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..0462919af660285 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -1061,18 +1061,20 @@ def Most : DiagGroup<"most", [
  ]>;
 
 // Thread Safety warnings
-def ThreadSafetyAttributes : DiagGroup<"thread-safety-attributes">;
-def ThreadSafetyAnalysis   : DiagGroup<"thread-safety-analysis">;
-def ThreadSafetyPrecise: DiagGroup<"thread-safety-precise">;
-def ThreadSafetyReference  : DiagGroup<"thread-safety-reference">;
-def ThreadSafetyNegative   : DiagGroup<"thread-safety-negative">;
+def ThreadSafetyAttributes   : DiagGroup<"thread-safety-attributes">;
+def ThreadSafetyAnalysis : DiagGroup<"thread-safety-analysis">;
+def ThreadSafetyPrecise  : DiagGroup<"thread-safety-precise">;
+def ThreadSafetyReference: DiagGroup<"thread-safety-reference">;
+def ThreadSafetyReferenceReturn  : DiagGroup<"thread-safety-reference-return">;
+def ThreadSafetyNegative : DiagGroup<"thread-safety-negative">;
 def ThreadSafety : DiagGroup<"thread-safety",
  [ThreadSafetyAttributes,
   ThreadSafetyAnalysis,
   ThreadSafetyPrecise,
   ThreadSafetyReference]>;
 def ThreadSafetyVerbose : DiagGroup<"thread-safety-verbose">;
-def ThreadSafetyBeta : DiagGroup<"thread-safety-beta">;
+def ThreadSafetyBeta : DiagGroup<"thread-safety-beta",
+ [ThreadSafetyReferenceReturn]>;
 
 // Uniqueness Analysis warnings
 def Consumed   : DiagGroup<"consumed">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b211680a0e9b6e9..c777d73dd4a769b 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68394)

2023-10-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/68394
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reapply "[clang analysis][thread-safety] Handle return-by-reference..… (PR #68394)

2023-10-06 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/68394

…. (#67776)"

Now that `scudo` issues have been fixed (#68273).

This reverts commit 462bdd5bf0861a27f451f7917802a954e2046bc7.

>From 6bf1af81adef93f20f25db4023810fe7be750410 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 29 Sep 2023 14:14:29 +0200
Subject: [PATCH] Reapply "[clang analysis][thread-safety] Handle
 return-by-reference... (#67776)"

Now that `scudo` issues have been fixed (#68273).

This reverts commit 462bdd5bf0861a27f451f7917802a954e2046bc7.
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 5 files changed, 161 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 28f1db29e62fa91..e3cd49bcc9ecc6a 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3864,7 +3864,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3873,6 +3873,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const 
CapabilityExpr ) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
-if (!CurrentMethod)
+if (!isa_and_nonnull(CurrentFunction))
   return false;
 const ValueDecl *VD = P->clangDecl();
-return VD->getDeclContext() == CurrentMethod->getDeclContext();
+return VD->getDeclContext() == CurrentFunction->getDeclContext();
   }
 
   return false;
@@ -1541,6 +1541,8 @@ class BuildLockset : public 
ConstStmtVisitor {
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  // The fact set for the function on exit.
+  const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
   llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public 
ConstStmtVisitor {
 bool SkipFirstParam = false);
 
 public:
-  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo )
+  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo ,
+   const FactSet )
   : ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet),
-LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
+FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
+CtxIndex(Info.EntryIndex) {}
 
   void VisitUnaryOperator(const UnaryOperator *UO);
   void VisitBinaryOperator(const BinaryOperator *BO);
@@ -1577,6 +1581,7 @@ class BuildLockset : public

[clang] Revert "[clang analysis][thread-safety] Handle return-by-reference...… (PR #67795)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/67795
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Revert "[clang analysis][thread-safety] Handle return-by-reference...… (PR #67795)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/67795

… (#67776)"

This detects issues in `scudo`. Reverting until these are fixed.

```
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:74:12:
 error: returning variable 'QuarantineCache' by reference requires holding 
mutex 'Mutex' exclusively [-Werror,-Wthread-safety-reference]
   74 | return QuarantineCache;
  |^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:248:28:
 note: in instantiation of member function 
'scudo::TSD>::getQuarantineCache' requested here
  248 | Quarantine.drain(>getQuarantineCache(),
  |^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:57:15:
 note: in instantiation of member function 
'scudo::Allocator::commitBack' 
requested here
   57 | Instance->commitBack(this);
  |   ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172:27:
 note: in instantiation of member function 
'scudo::TSD>::commitBack' requested here
  172 |   TSDRegistryT::ThreadTSD.commitBack(Instance);
  |   ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:33:46:
 note: in instantiation of function template specialization 
'scudo::teardownThread>' requested here
   33 | CHECK_EQ(pthread_key_create(, 
teardownThread), 0);
  |  ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:42:5:
 note: in instantiation of member function 
'scudo::TSDRegistryExT>::init' requested here
   42 | init(Instance); // Sets Initialized.
  | ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:130:5:
 note: in instantiation of member function 
'scudo::TSDRegistryExT>::initOnceMaybe' requested here
  130 | initOnceMaybe(Instance);
  | ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:74:5:
 note: in instantiation of member function 
'scudo::TSDRegistryExT>::initThread' requested here
   74 | initThread(Instance, MinimalInit);
  | ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:221:17:
 note: in instantiation of member function 
'scudo::TSDRegistryExT>::initThreadMaybe' requested here
  221 | TSDRegistry.initThreadMaybe(this, MinimalInit);
  | ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:790:5:
 note: in instantiation of member function 
'scudo::Allocator::initThreadMaybe' 
requested here
  790 | initThreadMaybe();
  | ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/wrappers_c.inc:36:25:
 note: in instantiation of member function 
'scudo::Allocator::canReturnNull' 
requested here
   36 | if (SCUDO_ALLOCATOR.canReturnNull()) {
```

This reverts commit 6dd96d6e80e9b3679a6161c590c60e0e99549b89.

>From 462bdd5bf0861a27f451f7917802a954e2046bc7 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 29 Sep 2023 14:12:17 +0200
Subject: [PATCH] Revert "[clang analysis][thread-safety] Handle
 return-by-reference... (#67776)"

This detects issues in `scudo`. Reverting until these are fixed.

```
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:74:12:
 error: returning variable 'QuarantineCache' by reference requires holding 
mutex 'Mutex' exclusively [-Werror,-Wthread-safety-reference]
   74 | return QuarantineCache;
  |^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/combined.h:248:28:
 note: in instantiation of member function 
'scudo::TSD>::getQuarantineCache' requested here
  248 | Quarantine.drain(>getQuarantineCache(),
  |^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd.h:57:15:
 note: in instantiation of member function 
'scudo::Allocator::commitBack' 
requested here
   57 | Instance->commitBack(this);
  |   ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:172:27:
 note: in instantiation of member function 
'scudo::TSD>::commitBack' requested here
  172 |   TSDRegistryT::ThreadTSD.commitBack(Instance);
  |   ^
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/compiler-rt/lib/scudo/standalone/tsd_exclusive.h:33:46:
 note: in instantiation of function template specialization 
'scudo::teardownThread>' requested here
   33 | CHECK_EQ(pthread_key_create(, 
teardownThread), 0);
  |

[clang] [clang analysis][thread-safety] Handle return-by-reference... (PR #67776)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/67776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang analysis][thread-safety] Handle return-by-reference... (PR #67776)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/67776

>From 7212f4cff29784f9b874a2c52586792f02158200 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 29 Sep 2023 10:39:58 +0200
Subject: [PATCH] [clang analysis][thread-safety] Handle return-by-reference...

...of guarded variables, when the function is not marked as requiring locks:

```
class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo _ref_locked() {
MutexLock lock();
return foo;  // BAD
  }

  Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo;  // OK
  }
};
```

Review on Phabricator: https://reviews.llvm.org/D153131
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 5 files changed, 161 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 29362df68365350..39e6bc5a73e128c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3858,7 +3858,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3867,6 +3867,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const 
CapabilityExpr ) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
-if (!CurrentMethod)
+if (!isa_and_nonnull(CurrentFunction))
   return false;
 const ValueDecl *VD = P->clangDecl();
-return VD->getDeclContext() == CurrentMethod->getDeclContext();
+return VD->getDeclContext() == CurrentFunction->getDeclContext();
   }
 
   return false;
@@ -1541,6 +1541,8 @@ class BuildLockset : public 
ConstStmtVisitor {
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  // The fact set for the function on exit.
+  const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
   llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public 
ConstStmtVisitor {
 bool SkipFirstParam = false);
 
 public:
-  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo )
+  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo ,
+   const FactSet )
   : ConstStmtVisitor(), Analyzer(Anlzr), FSet(Info.EntrySet),
-LVarCtx(Info.EntryContext), CtxIndex(Info.EntryIndex) {}
+FunctionExitFSet(FunctionExitFSet), LVarCtx(Info.EntryContext),
+CtxIndex(Info.EntryIndex) {}
 
   void VisitUnaryOperator(const UnaryOperator *UO);
   void

[clang] [clang analysis][thread-safety] Handle return-by-reference... (PR #67776)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/67776

...of guarded variables, when the function is not marked as requiring locks:

```
class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo _ref_locked() {
MutexLock lock();
return foo;  // BAD
  }

  Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo;  // OK
  }
};
```

Review on Phabricator: https://reviews.llvm.org/D153131

>From e5f03f5650038c8c6488960f7c158746e11da687 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 29 Sep 2023 10:39:58 +0200
Subject: [PATCH] [clang analysis][thread-safety] Handle return-by-reference...

...of guarded variables, when the function is not marked as requiring locks:

```
class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo _ref_locked() {
MutexLock lock();
return foo;  // BAD
  }

  Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo;  // OK
  }
};
```

Review on Phabricator: https://reviews.llvm.org/D153131
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 5 files changed, 161 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 29362df68365350..39e6bc5a73e128c 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3858,7 +3858,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3867,6 +3867,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 58dd7113665b132..54d0e95c6bd79a2 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const 
CapabilityExpr ) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
-if (!CurrentMethod)
+if (!isa_and_nonnull(CurrentFunction))
   return false;
 const ValueDecl *VD = P->clangDecl();
-return VD->getDeclContext() == CurrentMethod->getDeclContext();
+return VD->getDeclContext() == CurrentFunction->getDeclContext();
   }
 
   return false;
@@ -1541,6 +1541,8 @@ class BuildLockset : public 
ConstStmtVisitor {
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  // The fact set for the function on exit.
+  const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
   llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public 
ConstStmtVisitor {
 bool SkipFirstParam = false);
 
 public:
-  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo )
+  BuildLockset(ThreadSafetyAnalyzer *Anlzr,

[clang] Reland "[clang analysis][NFCI] Preparatory work for D153131. (#67420)… (PR #67775)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/67775
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Reland "[clang analysis][NFCI] Preparatory work for D153131. (#67420)… (PR #67775)

2023-09-29 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/67775

…" (#67523)

Discussion in https://reviews.llvm.org/D153132.

This reverts commit f70377471c990aa567584ae429e77adc9a55491b.

>From efc22cc97bbc0238f9ab066132ba434d41677599 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 29 Sep 2023 10:03:29 +0200
Subject: [PATCH] Reland "[clang analysis][NFCI] Preparatory work for D153131.
 (#67420)" (#67523)

Discussion in https://reviews.llvm.org/D153132.

This reverts commit f70377471c990aa567584ae429e77adc9a55491b.
---
 clang/lib/Analysis/ThreadSafety.cpp | 129 +++-
 1 file changed, 70 insertions(+), 59 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 3e6ceb7d54c427a..58dd7113665b132 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1055,6 +1055,19 @@ class ThreadSafetyAnalyzer {
   }
 
   void runAnalysis(AnalysisDeclContext );
+
+  void warnIfMutexNotHeld(const FactSet , const NamedDecl *D,
+  const Expr *Exp, AccessKind AK, Expr *MutexExp,
+  ProtectedOperationKind POK, til::LiteralPtr *Self,
+  SourceLocation Loc);
+  void warnIfMutexHeld(const FactSet , const NamedDecl *D, const Expr 
*Exp,
+   Expr *MutexExp, til::LiteralPtr *Self,
+   SourceLocation Loc);
+
+  void checkAccess(const FactSet , const Expr *Exp, AccessKind AK,
+   ProtectedOperationKind POK);
+  void checkPtAccess(const FactSet , const Expr *Exp, AccessKind AK,
+ ProtectedOperationKind POK);
 };
 
 } // namespace
@@ -1534,16 +1547,15 @@ class BuildLockset : public 
ConstStmtVisitor {
   unsigned CtxIndex;
 
   // helper functions
-  void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
-  Expr *MutexExp, ProtectedOperationKind POK,
-  til::LiteralPtr *Self, SourceLocation Loc);
-  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
-   til::LiteralPtr *Self, SourceLocation Loc);
 
   void checkAccess(const Expr *Exp, AccessKind AK,
-   ProtectedOperationKind POK = POK_VarAccess);
+   ProtectedOperationKind POK = POK_VarAccess) {
+Analyzer->checkAccess(FSet, Exp, AK, POK);
+  }
   void checkPtAccess(const Expr *Exp, AccessKind AK,
- ProtectedOperationKind POK = POK_VarAccess);
+ ProtectedOperationKind POK = POK_VarAccess) {
+Analyzer->checkPtAccess(FSet, Exp, AK, POK);
+  }
 
   void handleCall(const Expr *Exp, const NamedDecl *D,
   til::LiteralPtr *Self = nullptr,
@@ -1571,17 +1583,14 @@ class BuildLockset : public 
ConstStmtVisitor {
 
 /// Warn if the LSet does not contain a lock sufficient to protect access
 /// of at least the passed in AccessKind.
-void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
-  AccessKind AK, Expr *MutexExp,
-  ProtectedOperationKind POK,
-  til::LiteralPtr *Self,
-  SourceLocation Loc) {
+void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
+const FactSet , const NamedDecl *D, const Expr *Exp, AccessKind AK,
+Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self,
+SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
-
-  CapabilityExpr Cp =
-  Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
+  CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
-warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
+warnInvalidLock(Handler, MutexExp, D, Exp, Cp.getKind());
 return;
   } else if (Cp.shouldIgnore()) {
 return;
@@ -1589,68 +1598,67 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl 
*D, const Expr *Exp,
 
   if (Cp.negative()) {
 // Negative capabilities act like locks excluded
-const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, !Cp);
+const FactEntry *LDat = FSet.findLock(FactMan, !Cp);
 if (LDat) {
-  Analyzer->Handler.handleFunExcludesLock(
-  Cp.getKind(), D->getNameAsString(), (!Cp).toString(), Loc);
+  Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
+(!Cp).toString(), Loc);
   return;
 }
 
 // If this does not refer to a negative capability in the same class,
 // then stop here.
-if (!Analyzer->inCurrentScope(Cp))
+if (!inCurrentScope(Cp))
   return;
 
 // Otherwise the negative requirement must be propagated to the caller.
-LDat = FSet.findLock(Analyzer->FactMan, Cp);
+LDat = FSet.findLock(FactMan, Cp);
 if (!LDat) {
-

[clang] Revert "[clang analysis][NFCI] Preparatory work for D153131. (#67420)" (PR #67523)

2023-09-27 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/67523
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Revert "[clang analysis][NFCI] Preparatory work for D153131. (#67420)" (PR #67523)

2023-09-27 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/67523

There was a misunderstanding as to whether we wanted those base NFC changes or 
not.

This reverts commit 166074eff2e9a5f79b791f1cc9b641a4e2968616.

>From c4904f5c3304d0117a21ec6650a260639901dcf9 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Wed, 27 Sep 2023 09:43:46 +0200
Subject: [PATCH] Revert "[clang analysis][NFCI] Preparatory work for D153131.
 (#67420)"

There was a misunderstanding as to whether we wanted those base NFC changes or 
not.

This reverts commit 166074eff2e9a5f79b791f1cc9b641a4e2968616.
---
 clang/lib/Analysis/ThreadSafety.cpp | 133 +---
 1 file changed, 61 insertions(+), 72 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index f160cf4d013c78d..3e6ceb7d54c427a 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1015,19 +1015,6 @@ class ThreadSafetyAnalyzer {
 
   BeforeSet *GlobalBeforeSet;
 
-  void warnIfMutexNotHeld(const FactSet , const NamedDecl *D,
-  const Expr *Exp, AccessKind AK, Expr *MutexExp,
-  ProtectedOperationKind POK, til::LiteralPtr *Self,
-  SourceLocation Loc);
-  void warnIfMutexHeld(const FactSet , const NamedDecl *D, const Expr 
*Exp,
-   Expr *MutexExp, til::LiteralPtr *Self,
-   SourceLocation Loc);
-
-  void checkAccess(const FactSet , const Expr *Exp, AccessKind AK,
-   ProtectedOperationKind POK);
-  void checkPtAccess(const FactSet , const Expr *Exp, AccessKind AK,
- ProtectedOperationKind POK);
-
 public:
   ThreadSafetyAnalyzer(ThreadSafetyHandler , BeforeSet* Bset)
   : Arena(), SxBuilder(Arena), Handler(H), GlobalBeforeSet(Bset) {}
@@ -1547,15 +1534,16 @@ class BuildLockset : public 
ConstStmtVisitor {
   unsigned CtxIndex;
 
   // helper functions
+  void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
+  Expr *MutexExp, ProtectedOperationKind POK,
+  til::LiteralPtr *Self, SourceLocation Loc);
+  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
+   til::LiteralPtr *Self, SourceLocation Loc);
 
   void checkAccess(const Expr *Exp, AccessKind AK,
-   ProtectedOperationKind POK = POK_VarAccess) {
-Analyzer->checkAccess(FSet, Exp, AK, POK);
-  }
+   ProtectedOperationKind POK = POK_VarAccess);
   void checkPtAccess(const Expr *Exp, AccessKind AK,
- ProtectedOperationKind POK = POK_VarAccess) {
-Analyzer->checkPtAccess(FSet, Exp, AK, POK);
-  }
+ ProtectedOperationKind POK = POK_VarAccess);
 
   void handleCall(const Expr *Exp, const NamedDecl *D,
   til::LiteralPtr *Self = nullptr,
@@ -1583,14 +1571,17 @@ class BuildLockset : public 
ConstStmtVisitor {
 
 /// Warn if the LSet does not contain a lock sufficient to protect access
 /// of at least the passed in AccessKind.
-void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
-const FactSet , const NamedDecl *D, const Expr *Exp, AccessKind AK,
-Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self,
-SourceLocation Loc) {
+void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
+  AccessKind AK, Expr *MutexExp,
+  ProtectedOperationKind POK,
+  til::LiteralPtr *Self,
+  SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
-  CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
+
+  CapabilityExpr Cp =
+  Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
-warnInvalidLock(Handler, MutexExp, D, Exp, Cp.getKind());
+warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
 return;
   } else if (Cp.shouldIgnore()) {
 return;
@@ -1598,67 +1589,68 @@ void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
 
   if (Cp.negative()) {
 // Negative capabilities act like locks excluded
-const FactEntry *LDat = FSet.findLock(FactMan, !Cp);
+const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, !Cp);
 if (LDat) {
-Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
-  (!Cp).toString(), Loc);
-return;
+  Analyzer->Handler.handleFunExcludesLock(
+  Cp.getKind(), D->getNameAsString(), (!Cp).toString(), Loc);
+  return;
 }
 
 // If this does not refer to a negative capability in the same class,
 // then stop here.
-if (!inCurrentScope(Cp))
-return;
+if (!Analyzer->inCurrentScope(Cp))
+  return;
 
 // Otherwise the negative requirement must be

[clang] [clang analysis][thread-safety] Handle return-by-reference... (PR #67428)

2023-09-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/67428

...of guarded variables, when the function is not marked as requiring locks:

```
class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo _ref_locked() {
MutexLock lock();
return foo;  // BAD
  }

  Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo;  // OK
  }
};
```

This is carried over from phabricator: https://reviews.llvm.org/D153131

>From c14b45843e0a72d96c4898fc7d7ee72bcd619988 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Tue, 26 Sep 2023 15:22:09 +0200
Subject: [PATCH] [clang analysis][thread-safety] Handle return-by-reference...

...of guarded variables, when the function is not marked as requiring locks:

```
class Return {
  Mutex mu;
  Foo foo GUARDED_BY(mu);

  Foo _ref_locked() {
MutexLock lock();
return foo;  // BAD
  }

  Foo _ref_locks_required() SHARED_LOCKS_REQUIRED(mu) {
return foo;  // OK
  }
};
```

This is carried over from phabricator: https://reviews.llvm.org/D153131
---
 .../clang/Analysis/Analyses/ThreadSafety.h|  8 +-
 .../clang/Basic/DiagnosticSemaKinds.td| 10 ++-
 clang/lib/Analysis/ThreadSafety.cpp   | 80 +--
 clang/lib/Sema/AnalysisBasedWarnings.cpp  | 12 +++
 .../SemaCXX/warn-thread-safety-analysis.cpp   | 79 ++
 5 files changed, 161 insertions(+), 28 deletions(-)

diff --git a/clang/include/clang/Analysis/Analyses/ThreadSafety.h 
b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
index 1808d1d71e05d2c..0866b09bab2995e 100644
--- a/clang/include/clang/Analysis/Analyses/ThreadSafety.h
+++ b/clang/include/clang/Analysis/Analyses/ThreadSafety.h
@@ -47,7 +47,13 @@ enum ProtectedOperationKind {
   POK_PassByRef,
 
   /// Passing a pt-guarded variable by reference.
-  POK_PtPassByRef
+  POK_PtPassByRef,
+
+  /// Returning a guarded variable by reference.
+  POK_ReturnByRef,
+
+  /// Returning a pt-guarded variable by reference.
+  POK_PtReturnByRef,
 };
 
 /// This enum distinguishes between different kinds of lock actions. For
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index f4eb02fd9570c2f..8e423ea7691de88 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -3858,7 +3858,7 @@ def warn_fun_requires_negative_cap : Warning<
   "calling function %0 requires negative capability '%1'">,
   InGroup, DefaultIgnore;
 
-// Thread safety warnings on pass by reference
+// Thread safety warnings on pass/return by reference
 def warn_guarded_pass_by_reference : Warning<
   "passing variable %1 by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
@@ -3867,6 +3867,14 @@ def warn_pt_guarded_pass_by_reference : Warning<
   "passing the value that %1 points to by reference requires holding %0 "
   "%select{'%2'|'%2' exclusively}3">,
   InGroup, DefaultIgnore;
+def warn_guarded_return_by_reference : Warning<
+  "returning variable %1 by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
+def warn_pt_guarded_return_by_reference : Warning<
+  "returning the value that %1 points to by reference requires holding %0 "
+  "%select{'%2'|'%2' exclusively}3">,
+  InGroup, DefaultIgnore;
 
 // Imprecise thread safety warnings
 def warn_variable_requires_lock : Warning<
diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index f160cf4d013c78d..77b12f750e18a45 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1008,7 +1008,7 @@ class ThreadSafetyAnalyzer {
   threadSafety::SExprBuilder SxBuilder;
 
   ThreadSafetyHandler 
-  const CXXMethodDecl *CurrentMethod = nullptr;
+  const FunctionDecl *CurrentFunction;
   LocalVariableMap LocalVarMap;
   FactManager FactMan;
   std::vector BlockInfo;
@@ -1243,10 +1243,10 @@ bool ThreadSafetyAnalyzer::inCurrentScope(const 
CapabilityExpr ) {
 
   // Members are in scope from methods of the same class.
   if (const auto *P = dyn_cast(SExp)) {
-if (!CurrentMethod)
+if (!isa_and_nonnull(CurrentFunction))
   return false;
 const ValueDecl *VD = P->clangDecl();
-return VD->getDeclContext() == CurrentMethod->getDeclContext();
+return VD->getDeclContext() == CurrentFunction->getDeclContext();
   }
 
   return false;
@@ -1541,6 +1541,8 @@ class BuildLockset : public 
ConstStmtVisitor {
 
   ThreadSafetyAnalyzer *Analyzer;
   FactSet FSet;
+  // The fact set for the function on exit.
+  const FactSet 
   /// Maps constructed objects to `this` placeholder prior to initialization.
   llvm::SmallDenseMap ConstructedObjects;
   LocalVariableMap::Context LVarCtx;
@@ -1566,9 +1568,11 @@ class BuildLockset : public 
ConstStmtVisitor {
 bool SkipFirstParam = false);
 
 public:
-  BuildLockset(ThreadSafetyAnalyzer *Anlzr, CFGBlockInfo )
+

[clang] [clang analysis][NFCI] Preparatory work for D153131. (PR #67420)

2023-09-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/67420
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang analysis][NFCI] Preparatory work for D153131. (PR #67420)

2023-09-26 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/67420

This was ported over from phabricator.

Review https://reviews.llvm.org/D153131.

>From 3581fc00f690194ac4bac631311ecdb18593b7ba Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Tue, 26 Sep 2023 14:02:44 +0200
Subject: [PATCH] [clang analysis][NFCI] Preparatory work for D153131.

This was ported over from phabricator.i
Review https://reviews.llvm.org/D153131.
---
 clang/lib/Analysis/ThreadSafety.cpp | 133 +++-
 1 file changed, 72 insertions(+), 61 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 3e6ceb7d54c427a..f160cf4d013c78d 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -1015,6 +1015,19 @@ class ThreadSafetyAnalyzer {
 
   BeforeSet *GlobalBeforeSet;
 
+  void warnIfMutexNotHeld(const FactSet , const NamedDecl *D,
+  const Expr *Exp, AccessKind AK, Expr *MutexExp,
+  ProtectedOperationKind POK, til::LiteralPtr *Self,
+  SourceLocation Loc);
+  void warnIfMutexHeld(const FactSet , const NamedDecl *D, const Expr 
*Exp,
+   Expr *MutexExp, til::LiteralPtr *Self,
+   SourceLocation Loc);
+
+  void checkAccess(const FactSet , const Expr *Exp, AccessKind AK,
+   ProtectedOperationKind POK);
+  void checkPtAccess(const FactSet , const Expr *Exp, AccessKind AK,
+ ProtectedOperationKind POK);
+
 public:
   ThreadSafetyAnalyzer(ThreadSafetyHandler , BeforeSet* Bset)
   : Arena(), SxBuilder(Arena), Handler(H), GlobalBeforeSet(Bset) {}
@@ -1534,16 +1547,15 @@ class BuildLockset : public 
ConstStmtVisitor {
   unsigned CtxIndex;
 
   // helper functions
-  void warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp, AccessKind AK,
-  Expr *MutexExp, ProtectedOperationKind POK,
-  til::LiteralPtr *Self, SourceLocation Loc);
-  void warnIfMutexHeld(const NamedDecl *D, const Expr *Exp, Expr *MutexExp,
-   til::LiteralPtr *Self, SourceLocation Loc);
 
   void checkAccess(const Expr *Exp, AccessKind AK,
-   ProtectedOperationKind POK = POK_VarAccess);
+   ProtectedOperationKind POK = POK_VarAccess) {
+Analyzer->checkAccess(FSet, Exp, AK, POK);
+  }
   void checkPtAccess(const Expr *Exp, AccessKind AK,
- ProtectedOperationKind POK = POK_VarAccess);
+ ProtectedOperationKind POK = POK_VarAccess) {
+Analyzer->checkPtAccess(FSet, Exp, AK, POK);
+  }
 
   void handleCall(const Expr *Exp, const NamedDecl *D,
   til::LiteralPtr *Self = nullptr,
@@ -1571,17 +1583,14 @@ class BuildLockset : public 
ConstStmtVisitor {
 
 /// Warn if the LSet does not contain a lock sufficient to protect access
 /// of at least the passed in AccessKind.
-void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
-  AccessKind AK, Expr *MutexExp,
-  ProtectedOperationKind POK,
-  til::LiteralPtr *Self,
-  SourceLocation Loc) {
+void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
+const FactSet , const NamedDecl *D, const Expr *Exp, AccessKind AK,
+Expr *MutexExp, ProtectedOperationKind POK, til::LiteralPtr *Self,
+SourceLocation Loc) {
   LockKind LK = getLockKindFromAccessKind(AK);
-
-  CapabilityExpr Cp =
-  Analyzer->SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
+  CapabilityExpr Cp = SxBuilder.translateAttrExpr(MutexExp, D, Exp, Self);
   if (Cp.isInvalid()) {
-warnInvalidLock(Analyzer->Handler, MutexExp, D, Exp, Cp.getKind());
+warnInvalidLock(Handler, MutexExp, D, Exp, Cp.getKind());
 return;
   } else if (Cp.shouldIgnore()) {
 return;
@@ -1589,68 +1598,67 @@ void BuildLockset::warnIfMutexNotHeld(const NamedDecl 
*D, const Expr *Exp,
 
   if (Cp.negative()) {
 // Negative capabilities act like locks excluded
-const FactEntry *LDat = FSet.findLock(Analyzer->FactMan, !Cp);
+const FactEntry *LDat = FSet.findLock(FactMan, !Cp);
 if (LDat) {
-  Analyzer->Handler.handleFunExcludesLock(
-  Cp.getKind(), D->getNameAsString(), (!Cp).toString(), Loc);
-  return;
+Handler.handleFunExcludesLock(Cp.getKind(), D->getNameAsString(),
+  (!Cp).toString(), Loc);
+return;
 }
 
 // If this does not refer to a negative capability in the same class,
 // then stop here.
-if (!Analyzer->inCurrentScope(Cp))
-  return;
+if (!inCurrentScope(Cp))
+return;
 
 // Otherwise the negative requirement must be propagated to the caller.
-LDat = FSet.findLock(Analyzer->FactMan, Cp);
+LDat = FSet.findLock(FactMan, Cp);
 if

[clang] [NFC] Preparatory work for D153131 (PR #66750)

2023-09-19 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/66750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [NFC] Preparatory work for D153131 (PR #66750)

2023-09-19 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/66750

None

>From 3679b5c95a8de88a5cabc9ecdacdbbfef9c68ea8 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Tue, 19 Sep 2023 10:19:49 +0200
Subject: [PATCH] [NFC] Preparatory work for D153131

---
 clang/lib/Analysis/ThreadSafety.cpp | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/clang/lib/Analysis/ThreadSafety.cpp 
b/clang/lib/Analysis/ThreadSafety.cpp
index 34260ac8f4e7d6f..3107d035254dde6 100644
--- a/clang/lib/Analysis/ThreadSafety.cpp
+++ b/clang/lib/Analysis/ThreadSafety.cpp
@@ -2265,8 +2265,11 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
   const PostOrderCFGView *SortedGraph = walker.getSortedGraph();
   PostOrderCFGView::CFGBlockSet VisitedBlocks(CFGraph);
 
+  CFGBlockInfo  = BlockInfo[CFGraph->getEntry().getBlockID()];
+  CFGBlockInfo= BlockInfo[CFGraph->getExit().getBlockID()];
+
   // Mark entry block as reachable
-  BlockInfo[CFGraph->getEntry().getBlockID()].Reachable = true;
+  Initial.Reachable = true;
 
   // Compute SSA names for local variables
   LocalVarMap.traverseCFG(CFGraph, SortedGraph, BlockInfo);
@@ -2282,8 +2285,8 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
   // to initial lockset. Also turn off checking for lock and unlock functions.
   // FIXME: is there a more intelligent way to check lock/unlock functions?
   if (!SortedGraph->empty() && D->hasAttrs()) {
-const CFGBlock *FirstBlock = *SortedGraph->begin();
-FactSet  = BlockInfo[FirstBlock->getBlockID()].EntrySet;
+assert(*SortedGraph->begin() == >getEntry());
+FactSet  = Initial.EntrySet;
 
 CapExprSet ExclusiveLocksToAdd;
 CapExprSet SharedLocksToAdd;
@@ -2455,15 +2458,12 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 }
   }
 
-  CFGBlockInfo *Initial = [CFGraph->getEntry().getBlockID()];
-  CFGBlockInfo *Final   = [CFGraph->getExit().getBlockID()];
-
   // Skip the final check if the exit block is unreachable.
-  if (!Final->Reachable)
+  if (!Final.Reachable)
 return;
 
   // By default, we expect all locks held on entry to be held on exit.
-  FactSet ExpectedExitSet = Initial->EntrySet;
+  FactSet ExpectedExitSet = Initial.EntrySet;
 
   // Adjust the expected exit set by adding or removing locks, as declared
   // by *-LOCK_FUNCTION and UNLOCK_FUNCTION.  The intersect below will then
@@ -2479,7 +2479,7 @@ void 
ThreadSafetyAnalyzer::runAnalysis(AnalysisDeclContext ) {
 ExpectedExitSet.removeLock(FactMan, Lock);
 
   // FIXME: Should we call this function for all blocks which exit the 
function?
-  intersectAndWarn(ExpectedExitSet, Final->ExitSet, Final->ExitLoc,
+  intersectAndWarn(ExpectedExitSet, Final.ExitSet, Final.ExitLoc,
LEK_LockedAtEndOfFunction, LEK_NotLockedAtEndOfFunction);
 
   Handler.leaveFunction(CurrentFunction);

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-transformer] Allow stencils to read from system headers. (PR #66480)

2023-09-18 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle closed 
https://github.com/llvm/llvm-project/pull/66480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-transformer] Allow stencils to read from system headers. (PR #66480)

2023-09-15 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/66480

>From b23472e55093ea1d48d11386cc4ced43f913c170 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 15 Sep 2023 10:42:16 +0200
Subject: [PATCH] [clang-transformer] Allow stencils to read from system
 headers.

We were previously checking that stencil input ranges were writable. It 
suffices for them to be readable.
---
 clang/include/clang/Tooling/Transformer/SourceCode.h | 6 ++
 clang/lib/Tooling/Transformer/SourceCode.cpp | 6 +++---
 clang/lib/Tooling/Transformer/Stencil.cpp| 9 +
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/clang/include/clang/Tooling/Transformer/SourceCode.h 
b/clang/include/clang/Tooling/Transformer/SourceCode.h
index 44a4749db74c96c..004c614b0b9d93c 100644
--- a/clang/include/clang/Tooling/Transformer/SourceCode.h
+++ b/clang/include/clang/Tooling/Transformer/SourceCode.h
@@ -91,6 +91,12 @@ StringRef getExtendedText(const T , tok::TokenKind Next,
 llvm::Error validateEditRange(const CharSourceRange ,
   const SourceManager );
 
+/// Determines whether \p Range is one that can be read from. If
+/// `AllowSystemHeaders` is false, a range that falls within a system header
+/// fails validation.
+llvm::Error validateRange(const CharSourceRange , const SourceManager 
,
+  bool AllowSystemHeaders);
+
 /// Attempts to resolve the given range to one that can be edited by a rewrite;
 /// generally, one that starts and ends within a particular file. If a value is
 /// returned, it satisfies \c validateEditRange.
diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp 
b/clang/lib/Tooling/Transformer/SourceCode.cpp
index 35edc261ef09670..30009537b5923ce 100644
--- a/clang/lib/Tooling/Transformer/SourceCode.cpp
+++ b/clang/lib/Tooling/Transformer/SourceCode.cpp
@@ -50,9 +50,9 @@ CharSourceRange 
clang::tooling::maybeExtendRange(CharSourceRange Range,
   return CharSourceRange::getTokenRange(Range.getBegin(), Tok.getLocation());
 }
 
-static llvm::Error validateRange(const CharSourceRange ,
- const SourceManager ,
- bool AllowSystemHeaders) {
+llvm::Error clang::tooling::validateRange(const CharSourceRange ,
+  const SourceManager ,
+  bool AllowSystemHeaders) {
   if (Range.isInvalid())
 return llvm::make_error(errc::invalid_argument,
  "Invalid range");
diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp 
b/clang/lib/Tooling/Transformer/Stencil.cpp
index f2c1b6f8520a8cb..d91c9e0a20cc1b0 100644
--- a/clang/lib/Tooling/Transformer/Stencil.cpp
+++ b/clang/lib/Tooling/Transformer/Stencil.cpp
@@ -229,8 +229,8 @@ class SelectorStencil : public StencilInterface {
   // Validate the original range to attempt to get a meaningful error
   // message. If it's valid, then something else is the cause and we just
   // return the generic failure message.
-  if (auto Err =
-  tooling::validateEditRange(*RawRange, *Match.SourceManager))
+  if (auto Err = tooling::validateRange(*RawRange, *Match.SourceManager,
+/*AllowSystemHeaders=*/true))
 return handleErrors(std::move(Err), [](std::unique_ptr E) 
{
   assert(E->convertToErrorCode() ==
  llvm::make_error_code(errc::invalid_argument) &&
@@ -245,8 +245,9 @@ class SelectorStencil : public StencilInterface {
   "selected range could not be resolved to a valid source range");
 }
 // Validate `Range`, because `makeFileCharRange` accepts some ranges that
-// `validateEditRange` rejects.
-if (auto Err = tooling::validateEditRange(Range, *Match.SourceManager))
+// `validateRange` rejects.
+if (auto Err = tooling::validateRange(Range, *Match.SourceManager,
+  /*AllowSystemHeaders=*/true))
   return joinErrors(
   llvm::createStringError(errc::invalid_argument,
   "selected range is not valid for editing"),

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-transformer] Allow stencils to read from system headers. (PR #66480)

2023-09-15 Thread Clement Courbet via cfe-commits



@@ -91,6 +91,10 @@ StringRef getExtendedText(const T , tok::TokenKind Next,
 llvm::Error validateEditRange(const CharSourceRange ,
   const SourceManager );
 
+/// Determines whether \p Range is one that can be read from.
+llvm::Error validateRange(const CharSourceRange , const SourceManager 
,
+  bool AllowSystemHeaders);

legrosbuffle wrote:

done

https://github.com/llvm/llvm-project/pull/66480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-transformer] Allow stencils to read from system headers. (PR #66480)

2023-09-15 Thread Clement Courbet via cfe-commits



@@ -230,7 +230,7 @@ class SelectorStencil : public StencilInterface {
   // message. If it's valid, then something else is the cause and we just
   // return the generic failure message.
   if (auto Err =
-  tooling::validateEditRange(*RawRange, *Match.SourceManager))
+  tooling::validateRange(*RawRange, *Match.SourceManager, true))

legrosbuffle wrote:

done

https://github.com/llvm/llvm-project/pull/66480
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-transformer] Allow stencils to read from system headers. (PR #66480)

2023-09-15 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle updated 
https://github.com/llvm/llvm-project/pull/66480

>From 312eeef4c301e1049f50436fa3aa8d070b5cb4e9 Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 15 Sep 2023 10:42:16 +0200
Subject: [PATCH] [clang-transformer] Allow stencils to read from system
 headers.

We were previously checking that stencil input ranges were writable. It 
suffices for them to be readable.
---
 clang/include/clang/Tooling/Transformer/SourceCode.h | 6 ++
 clang/lib/Tooling/Transformer/SourceCode.cpp | 6 +++---
 clang/lib/Tooling/Transformer/Stencil.cpp| 9 +
 3 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/clang/include/clang/Tooling/Transformer/SourceCode.h 
b/clang/include/clang/Tooling/Transformer/SourceCode.h
index 44a4749db74c96c..cbb92b30a100290 100644
--- a/clang/include/clang/Tooling/Transformer/SourceCode.h
+++ b/clang/include/clang/Tooling/Transformer/SourceCode.h
@@ -91,6 +91,12 @@ StringRef getExtendedText(const T , tok::TokenKind Next,
 llvm::Error validateEditRange(const CharSourceRange ,
   const SourceManager );
 
+/// Determines whether \p Range is one that can be read from. If
+// `AllowSystemHeaders` is false, a range that falls within a system header
+// fails validation.
+llvm::Error validateRange(const CharSourceRange , const SourceManager 
,
+  bool AllowSystemHeaders);
+
 /// Attempts to resolve the given range to one that can be edited by a rewrite;
 /// generally, one that starts and ends within a particular file. If a value is
 /// returned, it satisfies \c validateEditRange.
diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp 
b/clang/lib/Tooling/Transformer/SourceCode.cpp
index 35edc261ef09670..30009537b5923ce 100644
--- a/clang/lib/Tooling/Transformer/SourceCode.cpp
+++ b/clang/lib/Tooling/Transformer/SourceCode.cpp
@@ -50,9 +50,9 @@ CharSourceRange 
clang::tooling::maybeExtendRange(CharSourceRange Range,
   return CharSourceRange::getTokenRange(Range.getBegin(), Tok.getLocation());
 }
 
-static llvm::Error validateRange(const CharSourceRange ,
- const SourceManager ,
- bool AllowSystemHeaders) {
+llvm::Error clang::tooling::validateRange(const CharSourceRange ,
+  const SourceManager ,
+  bool AllowSystemHeaders) {
   if (Range.isInvalid())
 return llvm::make_error(errc::invalid_argument,
  "Invalid range");
diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp 
b/clang/lib/Tooling/Transformer/Stencil.cpp
index f2c1b6f8520a8cb..d91c9e0a20cc1b0 100644
--- a/clang/lib/Tooling/Transformer/Stencil.cpp
+++ b/clang/lib/Tooling/Transformer/Stencil.cpp
@@ -229,8 +229,8 @@ class SelectorStencil : public StencilInterface {
   // Validate the original range to attempt to get a meaningful error
   // message. If it's valid, then something else is the cause and we just
   // return the generic failure message.
-  if (auto Err =
-  tooling::validateEditRange(*RawRange, *Match.SourceManager))
+  if (auto Err = tooling::validateRange(*RawRange, *Match.SourceManager,
+/*AllowSystemHeaders=*/true))
 return handleErrors(std::move(Err), [](std::unique_ptr E) 
{
   assert(E->convertToErrorCode() ==
  llvm::make_error_code(errc::invalid_argument) &&
@@ -245,8 +245,9 @@ class SelectorStencil : public StencilInterface {
   "selected range could not be resolved to a valid source range");
 }
 // Validate `Range`, because `makeFileCharRange` accepts some ranges that
-// `validateEditRange` rejects.
-if (auto Err = tooling::validateEditRange(Range, *Match.SourceManager))
+// `validateRange` rejects.
+if (auto Err = tooling::validateRange(Range, *Match.SourceManager,
+  /*AllowSystemHeaders=*/true))
   return joinErrors(
   llvm::createStringError(errc::invalid_argument,
   "selected range is not valid for editing"),

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [clang-transformer] Allow stencils to read from system headers. (PR #66480)

2023-09-15 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle created 
https://github.com/llvm/llvm-project/pull/66480

We were previously checking that stencil input ranges were writable. It 
suffices for them to be readable.

>From d9f8e39bb042165b53ae3c070f96a5bfe994f9fd Mon Sep 17 00:00:00 2001
From: Clement Courbet 
Date: Fri, 15 Sep 2023 10:42:16 +0200
Subject: [PATCH] [clang-transformer] Allow stencils to read from system
 headers.

We were previously checking that stencil input ranges were writable. It 
suffices for them to be readable.
---
 clang/include/clang/Tooling/Transformer/SourceCode.h | 4 
 clang/lib/Tooling/Transformer/SourceCode.cpp | 6 +++---
 clang/lib/Tooling/Transformer/Stencil.cpp| 6 +++---
 3 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/clang/include/clang/Tooling/Transformer/SourceCode.h 
b/clang/include/clang/Tooling/Transformer/SourceCode.h
index 44a4749db74c96c..01d8ef05e5687e5 100644
--- a/clang/include/clang/Tooling/Transformer/SourceCode.h
+++ b/clang/include/clang/Tooling/Transformer/SourceCode.h
@@ -91,6 +91,10 @@ StringRef getExtendedText(const T , tok::TokenKind Next,
 llvm::Error validateEditRange(const CharSourceRange ,
   const SourceManager );
 
+/// Determines whether \p Range is one that can be read from.
+llvm::Error validateRange(const CharSourceRange , const SourceManager 
,
+  bool AllowSystemHeaders);
+
 /// Attempts to resolve the given range to one that can be edited by a rewrite;
 /// generally, one that starts and ends within a particular file. If a value is
 /// returned, it satisfies \c validateEditRange.
diff --git a/clang/lib/Tooling/Transformer/SourceCode.cpp 
b/clang/lib/Tooling/Transformer/SourceCode.cpp
index 35edc261ef09670..30009537b5923ce 100644
--- a/clang/lib/Tooling/Transformer/SourceCode.cpp
+++ b/clang/lib/Tooling/Transformer/SourceCode.cpp
@@ -50,9 +50,9 @@ CharSourceRange 
clang::tooling::maybeExtendRange(CharSourceRange Range,
   return CharSourceRange::getTokenRange(Range.getBegin(), Tok.getLocation());
 }
 
-static llvm::Error validateRange(const CharSourceRange ,
- const SourceManager ,
- bool AllowSystemHeaders) {
+llvm::Error clang::tooling::validateRange(const CharSourceRange ,
+  const SourceManager ,
+  bool AllowSystemHeaders) {
   if (Range.isInvalid())
 return llvm::make_error(errc::invalid_argument,
  "Invalid range");
diff --git a/clang/lib/Tooling/Transformer/Stencil.cpp 
b/clang/lib/Tooling/Transformer/Stencil.cpp
index f2c1b6f8520a8cb..0c2037a8ae6c013 100644
--- a/clang/lib/Tooling/Transformer/Stencil.cpp
+++ b/clang/lib/Tooling/Transformer/Stencil.cpp
@@ -230,7 +230,7 @@ class SelectorStencil : public StencilInterface {
   // message. If it's valid, then something else is the cause and we just
   // return the generic failure message.
   if (auto Err =
-  tooling::validateEditRange(*RawRange, *Match.SourceManager))
+  tooling::validateRange(*RawRange, *Match.SourceManager, true))
 return handleErrors(std::move(Err), [](std::unique_ptr E) 
{
   assert(E->convertToErrorCode() ==
  llvm::make_error_code(errc::invalid_argument) &&
@@ -245,8 +245,8 @@ class SelectorStencil : public StencilInterface {
   "selected range could not be resolved to a valid source range");
 }
 // Validate `Range`, because `makeFileCharRange` accepts some ranges that
-// `validateEditRange` rejects.
-if (auto Err = tooling::validateEditRange(Range, *Match.SourceManager))
+// `validateRange` rejects.
+if (auto Err = tooling::validateRange(Range, *Match.SourceManager, true))
   return joinErrors(
   llvm::createStringError(errc::invalid_argument,
   "selected range is not valid for editing"),

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-15 Thread Clement Courbet via cfe-commits


https://github.com/legrosbuffle approved this pull request.


https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-14 Thread Clement Courbet via cfe-commits



@@ -145,6 +145,110 @@ TEST(LlvmLibcTypeTraitsTest, integral_constant) {
   EXPECT_EQ((integral_constant::value), 4);
 }
 
+namespace invoke_detail {
+
+enum State { INIT = 0, A_APPLY_CALLED, B_APPLY_CALLED };
+
+struct A {
+  State state = INIT;
+  virtual ~A() {}
+  virtual void apply() { state = A_APPLY_CALLED; }
+};
+
+struct B : public A {
+  virtual ~B() {}
+  virtual void apply() { state = B_APPLY_CALLED; }
+};
+
+void free_function() {}
+int free_function_return_5() { return 5; }
+int free_function_passtrough(int value) { return value; }
+
+struct Delegate {
+  int (*ptr)(int) = _function_passtrough;
+};
+
+template  struct Tag {
+  static int value() { return tag; }
+};
+
+struct Functor {
+  auto operator()() & { return Tag<0>(); }
+  auto operator()() const & { return Tag<1>(); }
+  auto operator()() && { return Tag<2>(); }
+  auto operator()() const && { return Tag<3>(); }
+};
+
+} // namespace invoke_detail
+
+TEST(LlvmLibcTypeTraitsTest, invoke) {
+  using namespace invoke_detail;
+  { // member function call
+A a;
+EXPECT_EQ(a.state, INIT);
+invoke(::apply, a);
+EXPECT_EQ(a.state, A_APPLY_CALLED);
+  }
+  { // overriden member function call
+B b;
+EXPECT_EQ(b.state, INIT);
+invoke(::apply, b);
+EXPECT_EQ(b.state, B_APPLY_CALLED);
+  }
+  { // free function
+invoke(_function);
+EXPECT_EQ(invoke(_function_return_5), 5);
+EXPECT_EQ(invoke(_function_passtrough, 1), 1);
+  }
+  { // pointer member function call
+Delegate d;
+EXPECT_EQ(invoke(::ptr, d, 2), 2);
+  }
+  { // Functor with several ref qualifier

legrosbuffle wrote:

qualifiers

https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-14 Thread Clement Courbet via cfe-commits



@@ -145,6 +145,110 @@ TEST(LlvmLibcTypeTraitsTest, integral_constant) {
   EXPECT_EQ((integral_constant::value), 4);
 }
 
+namespace invoke_detail {
+
+enum State { INIT = 0, A_APPLY_CALLED, B_APPLY_CALLED };
+
+struct A {
+  State state = INIT;
+  virtual ~A() {}
+  virtual void apply() { state = A_APPLY_CALLED; }
+};
+
+struct B : public A {
+  virtual ~B() {}
+  virtual void apply() { state = B_APPLY_CALLED; }
+};
+
+void free_function() {}
+int free_function_return_5() { return 5; }
+int free_function_passtrough(int value) { return value; }
+
+struct Delegate {
+  int (*ptr)(int) = _function_passtrough;
+};
+
+template  struct Tag {
+  static int value() { return tag; }
+};
+
+struct Functor {
+  auto operator()() & { return Tag<0>(); }
+  auto operator()() const & { return Tag<1>(); }
+  auto operator()() && { return Tag<2>(); }
+  auto operator()() const && { return Tag<3>(); }
+};
+
+} // namespace invoke_detail
+
+TEST(LlvmLibcTypeTraitsTest, invoke) {
+  using namespace invoke_detail;
+  { // member function call
+A a;
+EXPECT_EQ(a.state, INIT);
+invoke(::apply, a);
+EXPECT_EQ(a.state, A_APPLY_CALLED);
+  }
+  { // overriden member function call
+B b;
+EXPECT_EQ(b.state, INIT);
+invoke(::apply, b);
+EXPECT_EQ(b.state, B_APPLY_CALLED);
+  }
+  { // free function
+invoke(_function);
+EXPECT_EQ(invoke(_function_return_5), 5);
+EXPECT_EQ(invoke(_function_passtrough, 1), 1);
+  }
+  { // pointer member function call
+Delegate d;
+EXPECT_EQ(invoke(::ptr, d, 2), 2);
+  }
+  { // Functor with several ref qualifier
+Functor f;
+const Functor cf;
+EXPECT_EQ(invoke(f).value(), 0);
+EXPECT_EQ(invoke(cf).value(), 1);
+EXPECT_EQ(invoke(move(f)).value(), 2);
+EXPECT_EQ(invoke(move(cf)).value(), 3);
+  }
+  { // lambda
+EXPECT_EQ(invoke([]() -> int { return 2; }), 2);
+EXPECT_EQ(invoke([](int value) -> int { return value; }, 1), 1);
+
+const auto lambda = [](int) { return 0; };
+EXPECT_EQ(invoke(lambda, 1), 0);
+  }
+}
+
+TEST(LlvmLibcTypeTraitsTest, invoke_result) {
+  using namespace invoke_detail;
+  EXPECT_TRUE((is_same_v, void>));
+  EXPECT_TRUE((is_same_v, void>));
+  EXPECT_TRUE((is_same_v, void>));
+  EXPECT_TRUE((is_same_v, int>));
+  EXPECT_TRUE((is_same_v, int>));
+  EXPECT_TRUE((
+  is_same_v, 
int>));
+  // Functor with several ref qualifier
+  EXPECT_TRUE((is_same_v, Tag<0>>));
+  EXPECT_TRUE((is_same_v, Tag<1>>));
+  EXPECT_TRUE((is_same_v, Tag<2>>));
+  EXPECT_TRUE((is_same_v, Tag<3>>));
+  {
+auto lambda = []() {};
+EXPECT_TRUE((is_same_v, void>));
+  }
+  {
+auto lambda = []() { return 0; };
+EXPECT_TRUE((is_same_v, int>));
+  }
+  {
+auto lambda = [](int) -> double { return 0; };
+EXPECT_TRUE((is_same_v, double>));
+  }
+}

legrosbuffle wrote:

We're missing tests for a functor with overloaded types:

```
auto lambda = [](auto a) { return a; };
EXPECT_TRUE((is_same_v, int>));
EXPECT_TRUE((is_same_v, double>));
```

maybe also with `auto&` and `auto&&`.



https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-14 Thread Clement Courbet via cfe-commits



@@ -145,6 +145,110 @@ TEST(LlvmLibcTypeTraitsTest, integral_constant) {
   EXPECT_EQ((integral_constant::value), 4);
 }
 
+namespace invoke_detail {
+
+enum State { INIT = 0, A_APPLY_CALLED, B_APPLY_CALLED };
+
+struct A {
+  State state = INIT;
+  virtual ~A() {}
+  virtual void apply() { state = A_APPLY_CALLED; }
+};
+
+struct B : public A {
+  virtual ~B() {}
+  virtual void apply() { state = B_APPLY_CALLED; }
+};
+
+void free_function() {}
+int free_function_return_5() { return 5; }
+int free_function_passtrough(int value) { return value; }
+
+struct Delegate {
+  int (*ptr)(int) = _function_passtrough;
+};
+
+template  struct Tag {
+  static int value() { return tag; }
+};
+
+struct Functor {
+  auto operator()() & { return Tag<0>(); }
+  auto operator()() const & { return Tag<1>(); }
+  auto operator()() && { return Tag<2>(); }
+  auto operator()() const && { return Tag<3>(); }
+};
+
+} // namespace invoke_detail
+
+TEST(LlvmLibcTypeTraitsTest, invoke) {
+  using namespace invoke_detail;
+  { // member function call
+A a;
+EXPECT_EQ(a.state, INIT);
+invoke(::apply, a);
+EXPECT_EQ(a.state, A_APPLY_CALLED);
+  }
+  { // overriden member function call
+B b;
+EXPECT_EQ(b.state, INIT);
+invoke(::apply, b);
+EXPECT_EQ(b.state, B_APPLY_CALLED);
+  }
+  { // free function
+invoke(_function);
+EXPECT_EQ(invoke(_function_return_5), 5);
+EXPECT_EQ(invoke(_function_passtrough, 1), 1);
+  }
+  { // pointer member function call
+Delegate d;
+EXPECT_EQ(invoke(::ptr, d, 2), 2);
+  }
+  { // Functor with several ref qualifier

legrosbuffle wrote:

qualifiers

https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-14 Thread Clement Courbet via cfe-commits



@@ -145,6 +145,110 @@ TEST(LlvmLibcTypeTraitsTest, integral_constant) {
   EXPECT_EQ((integral_constant::value), 4);
 }
 
+namespace invoke_detail {
+
+enum State { INIT = 0, A_APPLY_CALLED, B_APPLY_CALLED };
+
+struct A {
+  State state = INIT;
+  virtual ~A() {}
+  virtual void apply() { state = A_APPLY_CALLED; }
+};
+
+struct B : public A {
+  virtual ~B() {}
+  virtual void apply() { state = B_APPLY_CALLED; }
+};
+
+void free_function() {}
+int free_function_return_5() { return 5; }
+int free_function_passtrough(int value) { return value; }
+
+struct Delegate {
+  int (*ptr)(int) = _function_passtrough;
+};
+
+template  struct Tag {
+  static int value() { return tag; }
+};
+
+struct Functor {
+  auto operator()() & { return Tag<0>(); }
+  auto operator()() const & { return Tag<1>(); }
+  auto operator()() && { return Tag<2>(); }
+  auto operator()() const && { return Tag<3>(); }
+};
+
+} // namespace invoke_detail
+
+TEST(LlvmLibcTypeTraitsTest, invoke) {
+  using namespace invoke_detail;
+  { // member function call
+A a;
+EXPECT_EQ(a.state, INIT);
+invoke(::apply, a);
+EXPECT_EQ(a.state, A_APPLY_CALLED);
+  }
+  { // overriden member function call
+B b;
+EXPECT_EQ(b.state, INIT);
+invoke(::apply, b);
+EXPECT_EQ(b.state, B_APPLY_CALLED);
+  }
+  { // free function
+invoke(_function);
+EXPECT_EQ(invoke(_function_return_5), 5);
+EXPECT_EQ(invoke(_function_passtrough, 1), 1);
+  }
+  { // pointer member function call
+Delegate d;
+EXPECT_EQ(invoke(::ptr, d, 2), 2);
+  }
+  { // Functor with several ref qualifier
+Functor f;
+const Functor cf;
+EXPECT_EQ(invoke(f).value(), 0);
+EXPECT_EQ(invoke(cf).value(), 1);
+EXPECT_EQ(invoke(move(f)).value(), 2);
+EXPECT_EQ(invoke(move(cf)).value(), 3);
+  }
+  { // lambda
+EXPECT_EQ(invoke([]() -> int { return 2; }), 2);
+EXPECT_EQ(invoke([](int value) -> int { return value; }, 1), 1);
+
+const auto lambda = [](int) { return 0; };
+EXPECT_EQ(invoke(lambda, 1), 0);
+  }
+}
+
+TEST(LlvmLibcTypeTraitsTest, invoke_result) {
+  using namespace invoke_detail;
+  EXPECT_TRUE((is_same_v, void>));
+  EXPECT_TRUE((is_same_v, void>));
+  EXPECT_TRUE((is_same_v, void>));
+  EXPECT_TRUE((is_same_v, int>));
+  EXPECT_TRUE((is_same_v, int>));
+  EXPECT_TRUE((
+  is_same_v, 
int>));
+  // Functor with several ref qualifier
+  EXPECT_TRUE((is_same_v, Tag<0>>));
+  EXPECT_TRUE((is_same_v, Tag<1>>));
+  EXPECT_TRUE((is_same_v, Tag<2>>));
+  EXPECT_TRUE((is_same_v, Tag<3>>));
+  {
+auto lambda = []() {};
+EXPECT_TRUE((is_same_v, void>));
+  }
+  {
+auto lambda = []() { return 0; };
+EXPECT_TRUE((is_same_v, int>));
+  }
+  {
+auto lambda = [](int) -> double { return 0; };
+EXPECT_TRUE((is_same_v, double>));
+  }
+}

legrosbuffle wrote:

We're missing tests for a functor with overloaded types:

```
auto lambda = [](auto a) { return a; };
EXPECT_TRUE((is_same_v, int>));
EXPECT_TRUE((is_same_v, double>));
```

maybe also with `auto&` and `auto&&`.



https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-14 Thread Clement Courbet via cfe-commits



@@ -145,6 +145,110 @@ TEST(LlvmLibcTypeTraitsTest, integral_constant) {
   EXPECT_EQ((integral_constant::value), 4);
 }
 
+namespace invoke_detail {
+
+enum State { INIT = 0, A_APPLY_CALLED, B_APPLY_CALLED };
+
+struct A {
+  State state = INIT;
+  virtual ~A() {}
+  virtual void apply() { state = A_APPLY_CALLED; }
+};
+
+struct B : public A {
+  virtual ~B() {}
+  virtual void apply() { state = B_APPLY_CALLED; }
+};
+
+void free_function() {}
+int free_function_return_5() { return 5; }
+int free_function_passtrough(int value) { return value; }
+
+struct Delegate {
+  int (*ptr)(int) = _function_passtrough;
+};
+
+template  struct Tag {
+  static int value() { return tag; }

legrosbuffle wrote:

`static constexpr int value = tag;` ?

https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] [libc] Add invoke / invoke_result type traits (PR #65750)

2023-09-14 Thread Clement Courbet via cfe-commits



@@ -145,6 +145,110 @@ TEST(LlvmLibcTypeTraitsTest, integral_constant) {
   EXPECT_EQ((integral_constant::value), 4);
 }
 
+namespace invoke_detail {
+
+enum State { INIT = 0, A_APPLY_CALLED, B_APPLY_CALLED };
+
+struct A {
+  State state = INIT;
+  virtual ~A() {}
+  virtual void apply() { state = A_APPLY_CALLED; }
+};
+
+struct B : public A {
+  virtual ~B() {}
+  virtual void apply() { state = B_APPLY_CALLED; }
+};
+
+void free_function() {}
+int free_function_return_5() { return 5; }
+int free_function_passtrough(int value) { return value; }
+
+struct Delegate {
+  int (*ptr)(int) = _function_passtrough;
+};
+
+template  struct Tag {
+  static int value() { return tag; }

legrosbuffle wrote:

`static constexpr int value = tag;` ?

https://github.com/llvm/llvm-project/pull/65750
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 32ffc55 - [clang-tidy] Really fix rG9182c679dde7

2023-05-24 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2023-05-24T15:21:50+02:00
New Revision: 32ffc1feda4cf3eeec5740af5c5f386e584c

URL: 
https://github.com/llvm/llvm-project/commit/32ffc1feda4cf3eeec5740af5c5f386e584c
DIFF: 
https://github.com/llvm/llvm-project/commit/32ffc1feda4cf3eeec5740af5c5f386e584c.diff

LOG: [clang-tidy] Really fix  rG9182c679dde7

Correct link is clang-tidy/checks/performance/no-automatic-move

Added: 


Modified: 
clang-tools-extra/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 8365340921e6..980be4869ead 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -384,11 +384,11 @@ Changes in existing checks
   with attributes and to support nested inline namespace introduced in c++20.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
-  ` when warning 
would be
+  ` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
 - Improved :doc:`performance-no-automatic-move
-  `: warn on 
``const &&``
+  `: warn on ``const &&``
   constructors.
 
 Removed checks



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 62dc3ba - [clang-tidy]Fix rG9182c679dde7cb6480e66b9231a53d43ad03908b

2023-05-24 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2023-05-24T15:19:00+02:00
New Revision: 62dc3ba8442fa3f7003d46d2838307452a0391f4

URL: 
https://github.com/llvm/llvm-project/commit/62dc3ba8442fa3f7003d46d2838307452a0391f4
DIFF: 
https://github.com/llvm/llvm-project/commit/62dc3ba8442fa3f7003d46d2838307452a0391f4.diff

LOG: [clang-tidy]Fix rG9182c679dde7cb6480e66b9231a53d43ad03908b

Fix bad link to documentation.

Added: 


Modified: 
clang-tools-extra/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index d1fd47542bd5..8365340921e6 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -384,7 +384,7 @@ Changes in existing checks
   with attributes and to support nested inline namespace introduced in c++20.
 
 - Fixed a false positive in :doc:`performance-no-automatic-move
-  ` when warning would be
+  ` when warning 
would be
   emitted for a const local variable to which NRVO is applied.
 
 - Improved :doc:`performance-no-automatic-move



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 9182c67 - [clang-tidy]performance-no-automatic-move: fix false negative on `const T&&` ctors.

2023-05-24 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2023-05-24T15:05:39+02:00
New Revision: 9182c679dde7cb6480e66b9231a53d43ad03908b

URL: 
https://github.com/llvm/llvm-project/commit/9182c679dde7cb6480e66b9231a53d43ad03908b
DIFF: 
https://github.com/llvm/llvm-project/commit/9182c679dde7cb6480e66b9231a53d43ad03908b.diff

LOG: [clang-tidy]performance-no-automatic-move: fix false negative on `const 
T&&` ctors.

We were only handling `const T&`/`T&&` ctor pairs, and we were missing 
uref-based ctors.

Differential Revision: https://reviews.llvm.org/D151092

Added: 


Modified: 
clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp 
b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
index 42bf8ff831868..7022e9d784fa7 100644
--- a/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/NoAutomaticMoveCheck.cpp
@@ -48,15 +48,23 @@ void NoAutomaticMoveCheck::registerMatchers(MatchFinder 
*Finder) {
   hasParameter(0, hasType(rValueReferenceType(
   pointee(type(equalsBoundNode("SrcT")));
 
+  // A matcher for `DstT::DstT(const Src&&)`, which typically comes from an
+  // instantiation of `template  DstT::DstT(U&&)`.
+  const auto ConstRefRefCtor = cxxConstructorDecl(
+  parameterCountIs(1),
+  hasParameter(0,
+   hasType(rValueReferenceType(pointee(isConstQualified());
+
   Finder->addMatcher(
-  traverse(TK_AsIs,
-   returnStmt(hasReturnValue(
-   ignoringElidableConstructorCall(ignoringParenImpCasts(
-   cxxConstructExpr(
-   hasDeclaration(LValueRefCtor),
-   hasArgument(0, ignoringParenImpCasts(declRefExpr(
-  to(NonNrvoConstLocalVariable)
-   .bind("ctor_call")),
+  traverse(
+  TK_AsIs,
+  returnStmt(hasReturnValue(
+  ignoringElidableConstructorCall(ignoringParenImpCasts(
+  cxxConstructExpr(
+  hasDeclaration(anyOf(LValueRefCtor, ConstRefRefCtor)),
+  hasArgument(0, ignoringParenImpCasts(declRefExpr(
+ to(NonNrvoConstLocalVariable)
+  .bind("ctor_call")),
   this);
 }
 

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 7fe2b97de8643..d1fd47542bd51 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -387,6 +387,10 @@ Changes in existing checks
   ` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
+- Improved :doc:`performance-no-automatic-move
+  `: warn on 
``const &&``
+  constructors.
+
 Removed checks
 ^^
 

diff  --git 
a/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp 
b/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
index d365f7de8b7c1..1ef7d16e38393 100644
--- 
a/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
+++ 
b/clang-tools-extra/test/clang-tidy/checkers/performance/no-automatic-move.cpp
@@ -7,30 +7,42 @@ struct Obj {
   virtual ~Obj();
 };
 
-template 
-struct StatusOr {
-  StatusOr(const T &);
-  StatusOr(T &&);
-};
-
 struct NonTemplate {
   NonTemplate(const Obj &);
   NonTemplate(Obj &&);
 };
 
+template  struct TemplateCtorPair {
+  TemplateCtorPair(const T &);
+  TemplateCtorPair(T &);
+};
+
+template  struct UrefCtor {
+  template  UrefCtor(U &);
+};
+
 template 
 T Make();
 
-StatusOr PositiveStatusOrConstValue() {
+NonTemplate PositiveNonTemplate() {
   const Obj obj = Make();
-  return obj;
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents 
automatic move [performance-no-automatic-move]
+  return obj; // selects `NonTemplate(const Obj&)`
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents
+  // automatic move [performance-no-automatic-move]
 }
 
-NonTemplate PositiveNonTemplateConstValue() {
+TemplateCtorPair PositiveTemplateCtorPair() {
   const Obj obj = Make();
-  return obj;
-  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents 
automatic move [performance-no-automatic-move]
+  return obj; // selects `TemplateCtorPair(const T&)`
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: constness of 'obj' prevents
+  // automatic move [performance-no-automatic-move]
+}
+
+UrefCtor PositiveUrefCtor() {
+  const Obj obj = Make();
+  return obj; // selects `UrefCtor(const T&&)`
+  // CHECK-MESSAGES:

[clang-tools-extra] d880d4e - [doc][clang-tidy] Sort release notes by check name.

2023-04-07 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2023-04-07T16:11:37+02:00
New Revision: d880d4e7c228e27bd8040e7e2616cfd8c02678bf

URL: 
https://github.com/llvm/llvm-project/commit/d880d4e7c228e27bd8040e7e2616cfd8c02678bf
DIFF: 
https://github.com/llvm/llvm-project/commit/d880d4e7c228e27bd8040e7e2616cfd8c02678bf.diff

LOG: [doc][clang-tidy] Sort release notes by check name.

Added: 


Modified: 
clang-tools-extra/docs/ReleaseNotes.rst

Removed: 




diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index facc7c78ea21..c10c6fd9d93d 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,6 +170,10 @@ Changes in existing checks
   ` check.
   Global options of the same name should be used instead.
 
+- Improved :doc:`bugprone-fold-init-type
+  ` to handle iterators that do not
+  define `value_type` type aliases.
+
 - Deprecated check-local options `HeaderFileExtensions` and 
`ImplementationFileExtensions`
   in :doc:`bugprone-suspicious-include
   ` check.
@@ -291,10 +295,6 @@ Changes in existing checks
   ` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
-- Improved :doc:`bugprone-fold-init-type
-  ` to handle iterators that do not
-  define `value_type` type aliases.
-
 Removed checks
 ^^
 



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] 1012677 - [clang-tidy]bugprone-fold-init-type

2023-04-07 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2023-04-07T14:17:03+02:00
New Revision: 1012677284b87091f76542c3c79da641101578ae

URL: 
https://github.com/llvm/llvm-project/commit/1012677284b87091f76542c3c79da641101578ae
DIFF: 
https://github.com/llvm/llvm-project/commit/1012677284b87091f76542c3c79da641101578ae.diff

LOG: [clang-tidy]bugprone-fold-init-type

Handle iterators that do not define a `value_type` type alias.

Get the value type from the return type of `Iter::operator*` instead.

Differential Revision: https://reviews.llvm.org/D137782

Added: 


Modified: 
clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp 
b/clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp
index c7a3032f27c03..d70cd2836c80f 100644
--- a/clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/FoldInitTypeCheck.cpp
@@ -24,9 +24,24 @@ void FoldInitTypeCheck::registerMatchers(MatchFinder 
*Finder) {
 return anyOf(
 // Pointer types.
 pointsTo(BuiltinTypeWithId(ID)),
-// Iterator types.
-recordType(hasDeclaration(has(typedefNameDecl(
-hasName("value_type"), hasType(BuiltinTypeWithId(ID)));
+// Iterator types have an `operator*` whose return type is the type we
+// care about.
+// Notes:
+//   - `operator*` can be in one of the bases of the iterator class.
+//   - this does not handle cases when the `operator*` is defined
+// outside the iterator class.
+recordType(
+hasDeclaration(cxxRecordDecl(isSameOrDerivedFrom(has(functionDecl(
+hasOverloadedOperatorName("*"),
+returns(qualType(hasCanonicalType(anyOf(
+// `value_type& operator*();`
+references(BuiltinTypeWithId(ID)),
+// `value_type operator*();`
+BuiltinTypeWithId(ID),
+// `auto operator*();`, `decltype(auto) operator*();`
+autoType(hasDeducedType(BuiltinTypeWithId(ID)))
+//
+)));
   };
 
   const auto IteratorParam = parmVarDecl(

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 6a87bcb3903e6..facc7c78ea21b 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -291,6 +291,10 @@ Changes in existing checks
   ` when warning would be
   emitted for a const local variable to which NRVO is applied.
 
+- Improved :doc:`bugprone-fold-init-type
+  ` to handle iterators that do not
+  define `value_type` type aliases.
+
 Removed checks
 ^^
 

diff  --git 
a/clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp 
b/clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp
index e7fdf395c50b3..2a49960e02895 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/fold-init-type.cpp
@@ -1,39 +1,67 @@
-// RUN: %check_clang_tidy %s bugprone-fold-init-type %t
+// RUN: %check_clang_tidy %s bugprone-fold-init-type -std=c++17 %t
 
 namespace std {
 template 
-T accumulate(InputIt first, InputIt last, T init);
+T accumulate(InputIt first, InputIt last, T init) {
+  // When `InputIt::operator*` returns a deduced `auto` type that refers to a
+  // dependent type, the return type is deduced only if `InputIt::operator*`
+  // is instantiated. In practice this happens somewhere in the implementation
+  // of `accumulate`. For tests, do it here.
+  (void)*first;
+}
 
 template 
-T reduce(InputIt first, InputIt last, T init);
+T reduce(InputIt first, InputIt last, T init) { (void)*first; }
 template 
 T reduce(ExecutionPolicy &,
- InputIt first, InputIt last, T init);
+ InputIt first, InputIt last, T init) { (void)*first; }
 
 struct parallel_execution_policy {};
 constexpr parallel_execution_policy par{};
 
 template 
 T inner_product(InputIt1 first1, InputIt1 last1,
-InputIt2 first2, T value);
+InputIt2 first2, T value) { (void)*first1; (void)*first2; }
 
 template 
 T inner_product(ExecutionPolicy &, InputIt1 first1, InputIt1 last1,
-InputIt2 first2, T value);
+InputIt2 first2, T value) { (void)*first1; (void)*first2; }
 
 } // namespace std
 
 struct FloatIterator {
-  typedef float value_type;
+  const float *() const;
 };
+
+struct DerivedFloatIterator : public FloatIterator {
+};
+
+template  struct ByValueTemplateIterator {
+  ValueType operator*() const;
+};
+
+template  struct ByRefTemplateIterator {
+  ValueType *();
+};
+

[clang] dbfa97b - [doc] Fix invalid reference to `hasReturnArgument` matcher.

2022-10-18 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2022-10-18T08:47:21+02:00
New Revision: dbfa97bd1108fe2a8e4fb13a16508f3dd0676a9b

URL: 
https://github.com/llvm/llvm-project/commit/dbfa97bd1108fe2a8e4fb13a16508f3dd0676a9b
DIFF: 
https://github.com/llvm/llvm-project/commit/dbfa97bd1108fe2a8e4fb13a16508f3dd0676a9b.diff

LOG: [doc] Fix invalid reference to `hasReturnArgument` matcher.

The matcher is called `hasReturnValue`.

Added: 


Modified: 
clang/docs/LibASTMatchersReference.html

Removed: 




diff  --git a/clang/docs/LibASTMatchersReference.html 
b/clang/docs/LibASTMatchersReference.html
index 4ac9ab0090cd4..61844ffc9de90 100644
--- a/clang/docs/LibASTMatchersReference.html
+++ b/clang/docs/LibASTMatchersReference.html
@@ -112,7 +112,7 @@ Traverse Mode
 traverse() matcher is used to set the mode:
 
 Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
-  returnStmt(hasReturnArgument(integerLiteral(equals(0
+  returnStmt(hasReturnValue(integerLiteral(equals(0
   ), this);
 
 



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 672311b - [CFG] Fix crash on CFG building when deriving from a template.

2022-08-16 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2022-08-16T13:01:13+02:00
New Revision: 672311bd77c594888e2660c124d7eae01822fffa

URL: 
https://github.com/llvm/llvm-project/commit/672311bd77c594888e2660c124d7eae01822fffa
DIFF: 
https://github.com/llvm/llvm-project/commit/672311bd77c594888e2660c124d7eae01822fffa.diff

LOG: [CFG] Fix crash on CFG building when deriving from a template.

Differential Revision: https://reviews.llvm.org/D121365

Added: 


Modified: 
clang/lib/Analysis/CFG.cpp
clang/unittests/Analysis/CFGBuildResult.h
clang/unittests/Analysis/CFGTest.cpp

Removed: 




diff  --git a/clang/lib/Analysis/CFG.cpp b/clang/lib/Analysis/CFG.cpp
index 5c45264896027..2b99b8e680805 100644
--- a/clang/lib/Analysis/CFG.cpp
+++ b/clang/lib/Analysis/CFG.cpp
@@ -1891,7 +1891,7 @@ void CFGBuilder::addImplicitDtorsForDestructor(const 
CXXDestructorDecl *DD) {
 // (which is 
diff erent from the current class) is responsible for
 // destroying them.
 const CXXRecordDecl *CD = VI.getType()->getAsCXXRecordDecl();
-if (!CD->hasTrivialDestructor()) {
+if (CD && !CD->hasTrivialDestructor()) {
   autoCreateBlock();
   appendBaseDtor(Block, );
 }
@@ -1901,7 +1901,7 @@ void CFGBuilder::addImplicitDtorsForDestructor(const 
CXXDestructorDecl *DD) {
   for (const auto  : RD->bases()) {
 if (!BI.isVirtual()) {
   const CXXRecordDecl *CD = BI.getType()->getAsCXXRecordDecl();
-  if (!CD->hasTrivialDestructor()) {
+  if (CD && !CD->hasTrivialDestructor()) {
 autoCreateBlock();
 appendBaseDtor(Block, );
   }

diff  --git a/clang/unittests/Analysis/CFGBuildResult.h 
b/clang/unittests/Analysis/CFGBuildResult.h
index 4851d3d7fb6d6..72ad1cc7ce401 100644
--- a/clang/unittests/Analysis/CFGBuildResult.h
+++ b/clang/unittests/Analysis/CFGBuildResult.h
@@ -56,13 +56,15 @@ class CFGCallback : public 
ast_matchers::MatchFinder::MatchCallback {
 TheBuildResult = BuildResult::SawFunctionBody;
 Options.AddImplicitDtors = true;
 if (std::unique_ptr Cfg =
-CFG::buildCFG(nullptr, Body, Result.Context, Options))
+CFG::buildCFG(Func, Body, Result.Context, Options))
   TheBuildResult = {BuildResult::BuiltCFG, Func, std::move(Cfg),
 std::move(AST)};
   }
 };
 
-inline BuildResult BuildCFG(const char *Code, CFG::BuildOptions Options = {}) {
+template 
+BuildResult BuildCFG(const char *Code, CFG::BuildOptions Options = {},
+ FuncMatcherT FuncMatcher = ast_matchers::anything()) {
   std::vector Args = {"-std=c++11",
"-fno-delayed-template-parsing"};
   std::unique_ptr AST = tooling::buildASTFromCodeWithArgs(Code, Args);
@@ -72,7 +74,8 @@ inline BuildResult BuildCFG(const char *Code, 
CFG::BuildOptions Options = {}) {
   CFGCallback Callback(std::move(AST));
   Callback.Options = Options;
   ast_matchers::MatchFinder Finder;
-  Finder.addMatcher(ast_matchers::functionDecl().bind("func"), );
+  Finder.addMatcher(ast_matchers::functionDecl(FuncMatcher).bind("func"),
+);
 
   Finder.matchAST(Callback.AST->getASTContext());
   return std::move(Callback.TheBuildResult);

diff  --git a/clang/unittests/Analysis/CFGTest.cpp 
b/clang/unittests/Analysis/CFGTest.cpp
index 1cce8bade42fe..7ce35e3fe4a4f 100644
--- a/clang/unittests/Analysis/CFGTest.cpp
+++ b/clang/unittests/Analysis/CFGTest.cpp
@@ -70,6 +70,27 @@ TEST(CFG, VariableOfIncompleteType) {
   EXPECT_EQ(BuildResult::BuiltCFG, BuildCFG(Code).getStatus());
 }
 
+// Constructing a CFG with a dependent base should not crash.
+TEST(CFG, DependantBaseAddImplicitDtors) {
+  const char *Code = R"(
+template 
+struct Base {
+  virtual ~Base() {}
+};
+
+template 
+struct Derived : public Base {
+  virtual ~Derived() {}
+};
+  )";
+  CFG::BuildOptions Options;
+  Options.AddImplicitDtors = true;
+  Options.setAllAlwaysAdd();
+  EXPECT_EQ(BuildResult::BuiltCFG,
+BuildCFG(Code, Options, ast_matchers::hasName("~Derived"))
+.getStatus());
+}
+
 TEST(CFG, IsLinear) {
   auto expectLinear = [](bool IsLinear, const char *Code) {
 BuildResult B = BuildCFG(Code);



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 156c075 - [clang][transformer] Finish plumbing `Note` all the way to the output.

2022-08-11 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2022-08-11T07:54:44+02:00
New Revision: 156c0754bc2ee0b7d198cfeff230374a1961ac04

URL: 
https://github.com/llvm/llvm-project/commit/156c0754bc2ee0b7d198cfeff230374a1961ac04
DIFF: 
https://github.com/llvm/llvm-project/commit/156c0754bc2ee0b7d198cfeff230374a1961ac04.diff

LOG: [clang][transformer] Finish plumbing `Note` all the way to the output.

Right now we can only add a single warning, notes are not possible.

Apparently some provisions were made to allow notes, but they were never
propagated all the way to the diagnostics.

Differential Revision: https://reviews.llvm.org/D128807

Added: 


Modified: 
clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
clang/include/clang/Tooling/Transformer/RewriteRule.h
clang/lib/Tooling/Transformer/RewriteRule.cpp

Removed: 




diff  --git a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp 
b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
index bd76e67f12c85..136b616836f1e 100644
--- a/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
+++ b/clang-tools-extra/clang-tidy/utils/TransformerClangTidyCheck.cpp
@@ -7,6 +7,7 @@
 
//===--===//
 
 #include "TransformerClangTidyCheck.h"
+#include "clang/Basic/DiagnosticIDs.h"
 #include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/STLExtras.h"
 
@@ -126,18 +127,28 @@ void TransformerClangTidyCheck::check(
   }
 
   // Associate the diagnostic with the location of the first change.
-  DiagnosticBuilder Diag =
-  diag((*Edits)[0].Range.getBegin(), escapeForDiagnostic(*Explanation));
-  for (const auto  : *Edits)
-switch (T.Kind) {
-case transformer::EditKind::Range:
-  Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
-  break;
-case transformer::EditKind::AddInclude:
-  Diag << Inserter.createIncludeInsertion(
-  Result.SourceManager->getFileID(T.Range.getBegin()), T.Replacement);
-  break;
+  {
+DiagnosticBuilder Diag =
+diag((*Edits)[0].Range.getBegin(), escapeForDiagnostic(*Explanation));
+for (const auto  : *Edits) {
+  switch (T.Kind) {
+  case transformer::EditKind::Range:
+Diag << FixItHint::CreateReplacement(T.Range, T.Replacement);
+break;
+  case transformer::EditKind::AddInclude:
+Diag << Inserter.createIncludeInsertion(
+Result.SourceManager->getFileID(T.Range.getBegin()), 
T.Replacement);
+break;
+  }
 }
+  }
+  // Emit potential notes.
+  for (const auto  : *Edits) {
+if (!T.Note.empty()) {
+  diag(T.Range.getBegin(), escapeForDiagnostic(T.Note),
+   DiagnosticIDs::Note);
+}
+  }
 }
 
 void TransformerClangTidyCheck::storeOptions(

diff  --git 
a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp 
b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
index 9106d5a77dd7d..5c5a0943c6cf8 100644
--- a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -28,6 +28,7 @@ using transformer::IncludeFormat;
 using transformer::makeRule;
 using transformer::node;
 using transformer::noopEdit;
+using transformer::note;
 using transformer::RewriteRuleWith;
 using transformer::RootID;
 using transformer::statement;
@@ -85,6 +86,7 @@ TEST(TransformerClangTidyCheckTest, 
DiagnosticsCorrectlyGenerated) {
   EXPECT_EQ(Errors.size(), 1U);
   EXPECT_EQ(Errors[0].Message.Message, "message");
   EXPECT_THAT(Errors[0].Message.Ranges, testing::IsEmpty());
+  EXPECT_THAT(Errors[0].Notes, testing::IsEmpty());
 
   // The diagnostic is anchored to the match, "return 5".
   EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
@@ -116,6 +118,27 @@ TEST(TransformerClangTidyCheckTest, EmptyReplacement) {
   EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
 }
 
+TEST(TransformerClangTidyCheckTest, NotesCorrectlyGenerated) {
+  class DiagAndNoteCheck : public TransformerClangTidyCheck {
+  public:
+DiagAndNoteCheck(StringRef Name, ClangTidyContext *Context)
+: TransformerClangTidyCheck(
+  makeRule(returnStmt(),
+   note(node(RootID), cat("some note")),
+   cat("message")),
+  Name, Context) {}
+  };
+  std::string Input = "int h() { return 5; }";
+  std::vector Errors;
+  EXPECT_EQ(Input, test::runCheckOnCode(Input, ));
+  EXPECT_EQ(Errors.size(), 1U);
+  EXPECT_EQ(Errors[0].Notes.size(), 1U);
+  EXPECT_EQ(Errors[0].Notes[0].Message, "some note");
+
+  // The note is anchored to the match, "return 5".
+  EXPECT_EQ(Errors[0].Notes[0].FileOffset, 10U);
+}
+
 TEST(TransformerClangTidyCheckTest, DiagnosticMessageEscaped) {
   class GiveDiagWithPercentSymbol :

[clang-tools-extra] 5331e12 - [clang][transformer] Fix crash on replacement-less ASTEdit.

2022-08-10 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2022-08-10T09:08:05+02:00
New Revision: 5331e1229aa6d0d33b5ec9fab7c05310187746d9

URL: 
https://github.com/llvm/llvm-project/commit/5331e1229aa6d0d33b5ec9fab7c05310187746d9
DIFF: 
https://github.com/llvm/llvm-project/commit/5331e1229aa6d0d33b5ec9fab7c05310187746d9.diff

LOG: [clang][transformer] Fix crash on replacement-less ASTEdit.

Given that we provide an EditGenerator edit(ASTEdit), we can't ever be
sure that the user won't give us an empty replacement.

Differential Revision: https://reviews.llvm.org/D128887

Added: 


Modified: 
clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
clang/lib/Tooling/Transformer/RewriteRule.cpp

Removed: 




diff  --git 
a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp 
b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
index a3600ab58ff3f..9106d5a77dd7d 100644
--- a/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
+++ b/clang-tools-extra/unittests/clang-tidy/TransformerClangTidyCheckTest.cpp
@@ -90,6 +90,32 @@ TEST(TransformerClangTidyCheckTest, 
DiagnosticsCorrectlyGenerated) {
   EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
 }
 
+transformer::ASTEdit noReplacementEdit(transformer::RangeSelector Target) {
+  transformer::ASTEdit E;
+  E.TargetRange = std::move(Target);
+  return E;
+}
+
+TEST(TransformerClangTidyCheckTest, EmptyReplacement) {
+  class DiagOnlyCheck : public TransformerClangTidyCheck {
+  public:
+DiagOnlyCheck(StringRef Name, ClangTidyContext *Context)
+: TransformerClangTidyCheck(
+  makeRule(returnStmt(), edit(noReplacementEdit(node(RootID))),
+   cat("message")),
+  Name, Context) {}
+  };
+  std::string Input = "int h() { return 5; }";
+  std::vector Errors;
+  EXPECT_EQ("int h() { }", test::runCheckOnCode(Input, 
));
+  EXPECT_EQ(Errors.size(), 1U);
+  EXPECT_EQ(Errors[0].Message.Message, "message");
+  EXPECT_THAT(Errors[0].Message.Ranges, testing::IsEmpty());
+
+  // The diagnostic is anchored to the match, "return 5".
+  EXPECT_EQ(Errors[0].Message.FileOffset, 10U);
+}
+
 TEST(TransformerClangTidyCheckTest, DiagnosticMessageEscaped) {
   class GiveDiagWithPercentSymbol : public TransformerClangTidyCheck {
   public:

diff  --git a/clang/lib/Tooling/Transformer/RewriteRule.cpp 
b/clang/lib/Tooling/Transformer/RewriteRule.cpp
index 3e76489782f34..822d00d33d164 100644
--- a/clang/lib/Tooling/Transformer/RewriteRule.cpp
+++ b/clang/lib/Tooling/Transformer/RewriteRule.cpp
@@ -50,17 +50,21 @@ translateEdits(const MatchResult , ArrayRef 
ASTEdits) {
 // produces a bad range, whereas the latter will simply ignore A.
 if (!EditRange)
   return SmallVector();
-auto Replacement = E.Replacement->eval(Result);
-if (!Replacement)
-  return Replacement.takeError();
-auto Metadata = E.Metadata(Result);
-if (!Metadata)
-  return Metadata.takeError();
 transformer::Edit T;
 T.Kind = E.Kind;
 T.Range = *EditRange;
-T.Replacement = std::move(*Replacement);
-T.Metadata = std::move(*Metadata);
+if (E.Replacement) {
+  auto Replacement = E.Replacement->eval(Result);
+  if (!Replacement)
+return Replacement.takeError();
+  T.Replacement = std::move(*Replacement);
+}
+if (E.Metadata) {
+  auto Metadata = E.Metadata(Result);
+  if (!Metadata)
+return Metadata.takeError();
+  T.Metadata = std::move(*Metadata);
+}
 Edits.push_back(std::move(T));
   }
   return Edits;



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] 90d2291 - [NFC] Fix comment.

2022-02-08 Thread Clement Courbet via cfe-commits


Author: Clement Courbet
Date: 2022-02-08T09:42:44+01:00
New Revision: 90d2291fbb4a382d8a36dfbb2bcfcc55f21b2bdc

URL: 
https://github.com/llvm/llvm-project/commit/90d2291fbb4a382d8a36dfbb2bcfcc55f21b2bdc
DIFF: 
https://github.com/llvm/llvm-project/commit/90d2291fbb4a382d8a36dfbb2bcfcc55f21b2bdc.diff

LOG: [NFC] Fix comment.

The extra space causes the table to render incorrectly in doxygen.

Added: 


Modified: 
clang/include/clang/Basic/Lambda.h

Removed: 




diff  --git a/clang/include/clang/Basic/Lambda.h 
b/clang/include/clang/Basic/Lambda.h
index 853821a33c2ad..de01d6f33c015 100644
--- a/clang/include/clang/Basic/Lambda.h
+++ b/clang/include/clang/Basic/Lambda.h
@@ -32,7 +32,7 @@ enum LambdaCaptureDefault {
 /// is an expression.
 enum LambdaCaptureKind {
   LCK_This,   ///< Capturing the \c *this object by reference
-  LCK_StarThis, /// < Capturing the \c *this object by copy
+  LCK_StarThis, ///< Capturing the \c *this object by copy
   LCK_ByCopy, ///< Capturing by copy (a.k.a., by value)
   LCK_ByRef,  ///< Capturing by reference
   LCK_VLAType ///< Capturing variable-length array type



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

1 2 3 >

1 - 100 of 215 matches

Mail list logo