[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In D80499#2362028 , @steveire wrote:

>> ! In D80499#2353187 , @alexfh wrote:
>>  You should be ready for back and forth with this change, if users hit 
>> widespread issues not caught by tests. Maybe splitting it into separate 
>> pieces and involving check authors where practical may be reasonable.
>
> Ok, I'll do that when https://reviews.llvm.org/D80961 and 
> https://reviews.llvm.org/D82278 are merged.

@alexfh This is done in D91302  and D91303 
. Could you review?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499

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

[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

> ! In D80499#2353187 , @alexfh wrote:
>  You should be ready for back and forth with this change, if users hit 
> widespread issues not caught by tests. Maybe splitting it into separate 
> pieces and involving check authors where practical may be reasonable.

Ok, I'll do that when https://reviews.llvm.org/D80961 and 
https://reviews.llvm.org/D82278 are merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499

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

[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In D80499#2352339 , @steveire wrote:

> @alexfh This change is based on the behavior of AST Matchers being changed to 
> ignore invisible/implicit AST nodes by default. As the default was not 
> changed in the end, this patch would need to be updated to add 
> `traverse(TK_IgnoreUnlessSpelledInSource)` wrapping around the matchers.
>
> Before I update the patch to add that, do you have any feedback?

If the default is not being changed, clear improvement will be noticeable only 
where multiple `ignore.*` matchers are being replaced with a 
`traverse(TK_IgnoreUnlessSpelledInSource)`. However, in many cases the behavior 
of the checks will change (and I'm not sure we have good enough test coverage 
especially where implicit conversions and similar are present). I believe, 
we're going to hit a number of corner cases and uncover a ton of bugs with this 
change. (Not that it would be different, if the default was changed to ignore 
implicit stuff ;)

You should be ready for back and forth with this change, if users hit 
widespread issues not caught by tests. Maybe splitting it into separate pieces 
and involving check authors where practical may be reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499

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

[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a subscriber: alexfh.
steveire added a comment.

@alexfh This change is based on the behavior of AST Matchers being changed to 
ignore invisible/implicit AST nodes by default. As the default was not changed 
in the end, this patch would need to be updated to add 
`traverse(TK_IgnoreUnlessSpelledInSource)` wrapping around the matchers.

Before I update the patch to add that, do you have any feedback?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499

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

[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

This is *great*, thank you for working on the cleanup! LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80499



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

[PATCH] D80499: Remove obsolete ignore*() matcher uses

[Stephen Kelly via Phabricator via cfe-commits]

steveire created this revision.
steveire added a reviewer: aaron.ballman.
Herald added subscribers: cfe-commits, arphaman, kbarton, nemanjai.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80499

Files:
  clang-tools-extra/clang-tidy/abseil/DurationAdditionCheck.cpp
  clang-tools-extra/clang-tidy/abseil/DurationConversionCastCheck.cpp
  clang-tools-extra/clang-tidy/abseil/DurationFactoryScaleCheck.cpp
  clang-tools-extra/clang-tidy/abseil/DurationRewriter.cpp
  clang-tools-extra/clang-tidy/abseil/DurationUnnecessaryConversionCheck.cpp
  clang-tools-extra/clang-tidy/abseil/StringFindStartswithCheck.cpp
  
clang-tools-extra/clang-tidy/bugprone/MisplacedPointerArithmeticInAllocCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MoveForwardingReferenceCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SizeofContainerCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SizeofExpressionCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/StringConstructorCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/StringIntegerAssignmentCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/StringLiteralWithEmbeddedNulCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousEnumUsageCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SuspiciousMissingCommaCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/TerminatingContinueCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsConstantArrayIndexCheck.cpp
  
clang-tools-extra/clang-tidy/cppcoreguidelines/ProBoundsPointerArithmeticCheck.cpp
  clang-tools-extra/clang-tidy/google/ExplicitMakePairCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/MultiwayPathsCoveredCheck.cpp
  clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
  clang-tools-extra/clang-tidy/misc/StaticAssertCheck.cpp
  clang-tools-extra/clang-tidy/misc/UnconventionalAssignOperatorCheck.cpp
  clang-tools-extra/clang-tidy/misc/UniqueptrResetReleaseCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseDefaultMemberInitCheck.cpp
  clang-tools-extra/clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp
  clang-tools-extra/clang-tidy/performance/FasterStringFindCheck.cpp
  
clang-tools-extra/clang-tidy/performance/InefficientStringConcatenationCheck.cpp
  clang-tools-extra/clang-tidy/performance/InefficientVectorOperationCheck.cpp
  clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
  clang-tools-extra/clang-tidy/readability/ElseAfterReturnCheck.cpp
  clang-tools-extra/clang-tidy/readability/RedundantSmartptrGetCheck.cpp
  clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp
  clang-tools-extra/clang-tidy/readability/SimplifySubscriptExprCheck.cpp
  clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp

Index: clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/UniqueptrDeleteReleaseCheck.cpp
@@ -27,11 +27,11 @@
  hasName("std::default_delete")));
 
   Finder->addMatcher(
-  cxxDeleteExpr(has(ignoringParenImpCasts(cxxMemberCallExpr(
-on(expr(hasType(UniquePtrWithDefaultDelete),
-unless(hasType(IsSusbstituted)))
-   .bind("uptr")),
-callee(cxxMethodDecl(hasName("release")))
+  cxxDeleteExpr(
+  has(cxxMemberCallExpr(on(expr(hasType(UniquePtrWithDefaultDelete),
+unless(hasType(IsSusbstituted)))
+   .bind("uptr")),
+callee(cxxMethodDecl(hasName("release"))
   .bind("delete"),
   this);
 }
Index: clang-tools-extra/clang-tidy/readability/SimplifySubscriptExprCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/SimplifySubscriptExprCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/SimplifySubscriptExprCheck.cpp
@@ -32,7 +32,7 @@
   llvm::SmallVector(Types.begin(), Types.end()));
 
   Finder->addMatcher(
-  arraySubscriptExpr(hasBase(ignoringParenImpCasts(
+  arraySubscriptExpr(hasBase(
   cxxMemberCallExpr(
   has(memberExpr().bind("member")),
   on(hasType(qualType(
@@ -40,7 +40,7 @@
hasDescendant(substTemplateTypeParmType(,
   anyOf(TypesMatcher, pointerType(pointee(TypesMatcher)),
   callee(namedDecl(hasName("data"
-  .bind("call",
+  .bind("call"))),
   this);
 }
 
Index: clang-tools-extra/clang-tidy/readability/SimplifyBooleanExprCheck.cpp