[PATCH] D103087: [clang-tidy] performances-unnecessary-* checks: Extend isOnlyUsedAsConst to expressions and catch const methods returning non-const references/pointers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

In D103087#2813901 , @flx wrote:

> In D103087#2793673 , @ymandel wrote:
>
>> I have some concerns about the cost of this checks as it used matching over 
>> entire contexts quite extensively.  At this point, the facilities involved 
>> seem quite close to doing dataflow analysis and I wonder if you might be 
>> better off with a very different implementation. Regardless, have you done 
>> any perfomance testing to see the impact on real code?
>
> That's a fair point. Is there prior art in terms of dataflow analysis in 
> ClangTidy or LLVM I could take a look at?

Added Dmitri to speak to prior art.

> In terms of measuring performance, do you have suggestions how to measure 
> this? I can add a counter that counts the recursion depth that is reached to 
> see how often this happens in practice.

I would simply run clang-tidy over a reasonable size set of files and see the 
timing w/ and w/o this change. But, I think that clang-tidy may have some 
built-in perf monitoring as well (specifically, a way to display the cost of 
each check, or the top most costly ones).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103087/new/

https://reviews.llvm.org/D103087

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

[PATCH] D103087: [clang-tidy] performances-unnecessary-* checks: Extend isOnlyUsedAsConst to expressions and catch const methods returning non-const references/pointers.

[Felix Berger via Phabricator via cfe-commits]

flx added a comment.

In D103087#2793673 , @ymandel wrote:

> I have some concerns about the cost of this checks as it used matching over 
> entire contexts quite extensively.  At this point, the facilities involved 
> seem quite close to doing dataflow analysis and I wonder if you might be 
> better off with a very different implementation. Regardless, have you done 
> any perfomance testing to see the impact on real code?

That's a fair point. Is there prior art in terms of dataflow analysis in 
ClangTidy or LLVM I could take a look at?

In terms of measuring performance, do you have suggestions how to measure this? 
I can add a counter that counts the recursion depth that is reached to see how 
often this happens in practice.

Another idea is to not count const methods from returning mutable pointer or 
reference types as const access. Standard types std::vector and absl::StatusOr 
would not be affected by this restriction, their const accessors return const 
references as well.

I'll hold off on this change until I see more false positives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103087/new/

https://reviews.llvm.org/D103087

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

[PATCH] D103087: [clang-tidy] performances-unnecessary-* checks: Extend isOnlyUsedAsConst to expressions and catch const methods returning non-const references/pointers.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel added a comment.

I have some concerns about the cost of this checks as it used matching over 
entire contexts quite extensively.  At this point, the facilities involved seem 
quite close to doing dataflow analysis and I wonder if you might be better off 
with a very different implementation. Regardless, have you done any perfomance 
testing to see the impact on real code?




Comment at: clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h:30
 
+// Returns true if expression is only used in a const compatible fashion.
+bool isOnlyUsedAsConst(const Expr , const Stmt , ASTContext );

please expand this comment. It's not obvious (to me) what it means for a 
variable to be used in a "const compatible fashion".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103087/new/

https://reviews.llvm.org/D103087

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

[PATCH] D103087: [clang-tidy] performances-unnecessary-* checks: Extend isOnlyUsedAsConst to expressions and catch const methods returning non-const references/pointers.

[Felix Berger via Phabricator via cfe-commits]

flx created this revision.
flx added reviewers: aaron.ballman, hokein, ymandel.
Herald added a subscriber: xazax.hun.
flx requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

This change extends isOnlyUsedAsConst() to avoid false positives in cases where
a const method returns a mutable pointer or refernce and the pointed to object
is modified.

To achieve this we look at each const method or operator call and check if it
returns a mutable pointer/reference. If that's the case the call expression
itself is checked for constant use.

We also capture assignments of expressions to non-const references/pointers and
then check that the declared alias variables are used in a const-only fasion as
well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103087

Files:
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.cpp
  clang-tools-extra/clang-tidy/utils/DeclRefExprUtils.h
  clang-tools-extra/clang-tidy/utils/Matchers.h
  
clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/performance-unnecessary-copy-initialization.cpp
@@ -6,6 +6,9 @@
   const ExpensiveToCopyType () const;
   void nonConstMethod();
   bool constMethod() const;
+  ExpensiveToCopyType *() const;
+  ExpensiveToCopyType *operator->() const;
+  ExpensiveToCopyType *get() const;
 };
 
 struct TrivialToCopyType {
@@ -508,3 +511,79 @@
   // CHECK-FIXES: const auto& UnnecessaryCopy = Ref.reference();
   Orig.constMethod();
 }
+
+void negativePointerIsModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  NecessaryCopy->nonConstMethod();
+}
+
+void negativeReferenceIsModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  (*NecessaryCopy).nonConstMethod();
+}
+
+void negativePointerIsModifiedThroughConstOperatorChain() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  (*NecessaryCopy)->nonConstMethod();
+}
+
+void positivePointerIsNotModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  UnnecessaryCopy->constMethod();
+}
+
+void positiveReferenceIsNotModifiedThroughConstOperator() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  (*UnnecessaryCopy).constMethod();
+}
+
+void negativeReferenceIsAssignedToVarAndModified() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  auto  = *NecessaryCopy;
+  Ref.nonConstMethod();
+}
+
+void positiveReferenceIsAssignedToVarAndNotModified() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  auto  = *UnnecessaryCopy;
+  Ref.constMethod();
+}
+
+void negativePointerIsAssignedToVarAndModified() {
+  ExpensiveToCopyType Orig;
+  auto NecessaryCopy = Orig;
+  auto *Pointer = NecessaryCopy.get();
+  Pointer->nonConstMethod();
+}
+
+void positivePointerIsAssignedToVarAndNotModified() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  auto *Pointer = UnnecessaryCopy.get();
+  Pointer->constMethod();
+}
+
+void positiveConstMethodReturningPointer() {
+  ExpensiveToCopyType Orig;
+  auto UnnecessaryCopy = Orig;
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: local copy 'UnnecessaryCopy' of the variable 'Orig' is never modified;
+  // CHECK-FIXES: const auto& UnnecessaryCopy = Orig;
+  // Test that a const method that returns a mutable pointer/reference that is
+  // unused counts as a const use.
+  UnnecessaryCopy.get();
+}
Index: clang-tools-extra/clang-tidy/utils/Matchers.h
===
--- clang-tools-extra/clang-tidy/utils/Matchers.h
+++ clang-tools-extra/clang-tidy/utils/Matchers.h
@@ -43,12 +43,24 @@
   return referenceType(pointee(qualType(isConstQualified(;
 }
 
+AST_MATCHER_FUNCTION(ast_matchers::TypeMatcher, isReferenceToNonConst) {
+  using namespace ast_matchers;
+  return allOf(referenceType(),
+