[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9ff4f2dfea63: [clang-tidy] Fix #55134 (regression introduced 
by 5da7c04) (authored by salman-javed-nz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   means it is advised to use YAML's block style initiated by the pipe 
character `|` for the `Checks`
   section in order to benefit from the easier syntax that works without commas.
 
+- Fixed a regression introduced in clang-tidy 14.0.0, which prevented NOLINTs
+  from suppressing diagnostics associated with macro arguments. This fixes
+  `Issue 55134 `_.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* 

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 431805.
salman-javed-nz added a comment.

Added release note.

(Also taking the opportunity to run the patch through the pre-merge build bot 
one last time.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -106,6 +106,10 @@
   means it is advised to use YAML's block style initiated by the pipe 
character `|` for the `Checks`
   section in order to benefit from the easier syntax that works without commas.
 
+- Fixed a regression introduced in clang-tidy 14.0.0, which prevented NOLINTs
+  from suppressing diagnostics associated with macro arguments. This fixes
+  `Issue 55134 `_.
+
 New checks
 ^^
 
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM, though this should have a release note for the fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Nathan Ridge via Phabricator via cfe-commits]

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

I'm not a clang-tidy maintainer so I don't know if I have the authority to 
green-light this patch, but I'm fairly confident the fix is correct (it's 
restoring the previous usage of the SourceManager API in this loop prior to 
this refactor 
),
 and the tests capture the changed behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Paul Altin via Phabricator via cfe-commits]

paulaltin added a comment.

In D126138#3530172 , @salman-javed-nz 
wrote:

> This fix is check-agnostic, so I don't think we need to add even more tests 
> than the two proposed here.

Yes, you're right; the issue itself is check-agnostic too, I just didn't 
realise that when I wrote the above comment (thanks to @nridge for explaining 
what's really going on).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D126138#3529820 , @paulaltin wrote:

> Thanks for preparing this revision @salman-javed-nz!
>
> Do you think it could be worth adding a few more test cases to cover this? It 
> turned out that this issue wasn't actually specific to multi-line macros (see 
> this comment 
> ), 
> so if the test case on line 96 was passing then it must not be 
> fully/correctly testing NOLINT for single-line macros. I guess the only way 
> to do this would be to add more checks to the nolint.cpp file, but I realise 
> that's not a trivial change.

I added a second test case to this patch, to cover the specific check mentioned 
in the GitHub ticket.
This fix is check-agnostic, so I don't think we need to add even more tests 
than the two proposed here.

I think I've addressed what everyone has discussed in this review up to this 
point. Let me know what other updates I can make to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 431223.
salman-javed-nz added a comment.

Add another test, to test the specific code sample in the GitHub issue 
(check=cppcoreguidelines-pro-type-member-init).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s 
google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init
 %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -1,5 +1,5 @@
 // REQUIRES: static-analyzer
-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult,modernize-avoid-c-arrays,cppcoreguidelines-avoid-c-arrays,cppcoreguidelines-pro-type-member-init %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
 
 #include "trigger_warning.h"
 void I(int& Out) {
@@ -96,6 +96,23 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_1(X)\
+  class X { \
+X(int i); /* NOLINT(google-explicit-constructor) */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_1(G1)
+
+#define MACRO_SUPPRESS_DIAG_FOR_ARG_2(X)  \
+  struct X { /* NOLINT(cppcoreguidelines-pro-type-member-init) */ \
+int a = 0;\
+int b;\
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG_2(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +133,4 @@
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: 

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

The pre-merge checks are failing on the clang-format check. Not sure why it's 
complaining about the formatting of the `lit` test files - those files have 
never complied with the project style and have not been checked for style for 
as long as I remember. Has this policy changed recently?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz updated this revision to Diff 431213.
salman-javed-nz edited the summary of this revision.
salman-javed-nz added a comment.

Renamed test case to better explain what it is that it's testing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,16 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG(X) \
+  class X {\
+X(int i); /* NOLINT */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG(G1)
+MACRO_SUPPRESS_DIAG_FOR_ARG(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +126,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,16 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+// Check that we can suppress diagnostics about macro arguments (as opposed to
+// diagnostics about the macro contents itself).
+#define MACRO_SUPPRESS_DIAG_FOR_ARG(X) \
+  class X {\
+X(int i); /* NOLINT */ \
+  };
+
+MACRO_SUPPRESS_DIAG_FOR_ARG(G1)
+MACRO_SUPPRESS_DIAG_FOR_ARG(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +126,4 @@
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D126138#3529820 , @paulaltin wrote:

> It turned out that this issue wasn't actually specific to multi-line macros 
> (see this comment 
> ),

Indeed, single-line vs. multi-line was a red herring. Rather, the issue was 
specific to cases where the diagnostic being suppressed was attached to a token 
that came from a macro **argument**, rather than elsewhere in the macro 
expansion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Paul Altin via Phabricator via cfe-commits]

paulaltin added a comment.

Thanks for preparing this revision @salman-javed-nz!

Do you think it could be worth adding a few more test cases to cover this? It 
turned out that this issue wasn't actually specific to multi-line macros (see 
this comment 
), 
so if the test case on line 96 was passing then it must not be fully/correctly 
testing NOLINT for single-line macros. I guess the only way to do this would be 
to add more checks to the nolint.cpp file, but I realise that's not a trivial 
change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

F23151067: Capture.png 
This screenshot shows the code snippet from the GitHub issue, and Clang-Tidy's 
behaviour before and after this fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126138

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

[PATCH] D126138: [clang-tidy] Fix #55134 (regression introduced by 5da7c04)

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz created this revision.
salman-javed-nz added reviewers: nridge, paulaltin.
salman-javed-nz added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
salman-javed-nz requested review of this revision.
Herald added a subscriber: cfe-commits.

5da7c04  
introduced a regression in the NOLINT macro checking loop, replacing the call 
to `getImmediateExpansionRange().getBegin()` with 
`getImmediateMacroCallerLoc()`, which has similar but subtly different 
behaviour.

The consequence is that NOLINTs within macros such as the example below were 
not being detected:

  #define CREATE_STRUCT(name)\
  struct name {/* NOLINT */  \
  int a = 0; \
  int b; \
  };
  
  CREATE_STRUCT(X)

Revert to pre-patch behaviour and add test cases to cover this issue.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D126138

Files:
  clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,13 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+#define MACRO_MULTILINE_NOLINT(X) \
+class X { \
+X(int i); /* NOLINT */\
+};
+MACRO_MULTILINE_NOLINT(G1)
+MACRO_MULTILINE_NOLINT(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +123,4 @@
 int array3[10];  // 
NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }


Index: clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/nolint.cpp
@@ -96,6 +96,13 @@
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
+#define MACRO_MULTILINE_NOLINT(X) \
+class X { \
+X(int i); /* NOLINT */\
+};
+MACRO_MULTILINE_NOLINT(G1)
+MACRO_MULTILINE_NOLINT(G2)
+
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
@@ -116,4 +123,4 @@
 int array3[10];  // NOLINT(cppcoreguidelines-avoid-c-arrays,modernize-avoid-c-arrays)
 int array4[10];  // NOLINT(*-avoid-c-arrays)
 
-// CHECK-MESSAGES: Suppressed 34 warnings (34 NOLINT)
+// CHECK-MESSAGES: Suppressed 36 warnings (36 NOLINT)
Index: clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
===
--- clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
+++ clang-tools-extra/clang-tidy/NoLintDirectiveHandler.cpp
@@ -266,7 +266,7 @@
   return true;
 if (!DiagLoc.isMacroID())
   return false;
-DiagLoc = SrcMgr.getImmediateMacroCallerLoc(DiagLoc);
+DiagLoc = SrcMgr.getImmediateExpansionRange(DiagLoc).getBegin();
   }
   return false;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits