[PATCH] D59540: [clang-tidy] [PR41119] readability-identifier-naming incorrectly fixes lambda capture

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Thanks, looks better now, but still a few comments, mostly nits.



================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:701
+  const ASTContext::DynTypedNodeList &Parents = Context->getParents(*DeclRef);
+  if (!Parents.empty()) {
+    const Stmt *ST = Parents[0].get<Stmt>();
----------------
Please use early return:

  if (Parents.empty())
    return false;


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:702
+  if (!Parents.empty()) {
+    const Stmt *ST = Parents[0].get<Stmt>();
+    // Ignore ImplicitCastExpr if we find one.
----------------
Same here:
  if (!ST)
    return false;


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:710
+    }
+    if (ST && isa<LambdaExpr>(ST))
+      return true;
----------------
And here just `return ST && isa<LambdaExpr>(ST);`


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:810
     SourceRange Range = DeclRef->getNameInfo().getSourceRange();
-    addUsage(NamingCheckFailures, DeclRef->getDecl(), Range,
-             Result.SourceManager);
-    return;
+    bool LambdaDeclRef = isLambda(DeclRef, Result.Context);
+    if (LambdaDeclRef) {
----------------
Let's drop this variable. The code is going to be simpler without it:

  if (isLambda(DeclRef, Result.Context)) {
    StringRef CaptureText = ...;
    if (CaptureText != "=" && CaptureText != "&") {
      addUsage(...);
      return;
    }
  }


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:812
+    if (LambdaDeclRef) {
+      std::string captureText =
+          Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
Lexer::getSourceText returns a StringRef, there's no need to copy its contents.


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:812
+    if (LambdaDeclRef) {
+      std::string captureText =
+          Lexer::getSourceText(CharSourceRange::getTokenRange(Range),
----------------
alexfh wrote:
> Lexer::getSourceText returns a StringRef, there's no need to copy its contents.
nit: CaptureText


================
Comment at: clang-tidy/readability/IdentifierNamingCheck.cpp:815
+                               *Result.SourceManager, getLangOpts());
+      if (!(captureText == "=" || captureText == "&"))
+        LambdaDeclRef = false;
----------------
I find it easier to understand, if negation is propagated through the parentheses:

  if (captureText != "=" && captureText != "&")



================
Comment at: test/clang-tidy/readability-identifier-naming.cpp:506
+bool Foo() {
+  bool Columns=false;
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
----------------
alexfh wrote:
> If the formatting is not critical for the logic of the test, please clang-format the new test code.
nit: The formatting is still off here, e.g. missing spaces around the equals sign.


================
Comment at: test/clang-tidy/readability-identifier-naming.cpp:508
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: invalid case style for local variable 'Columns'
+// CHECK-FIXES: {{^}}  bool columns=false;
+  auto ptr = [&]{
----------------
Let's make the CHECK-FIXES lines unique to reduce the chance of mixing them up and to make identifying broken test cases easier. One way to do this is to make the variable names distinct, another way is to add unique trailing comments both in the code and in the CHECK-FIXES patterns.


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

https://reviews.llvm.org/D59540