[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

In D116378#3226780 , @carlosgalvezp 
wrote:

> By the way, the similar problem exists in Clang compiler. I have written in 
> cfe-dev , 
> Discourse 
> 
>  and submitted an issue  
> without any feedback so far (I understand it's been vacation). Do you think 
> it would be beneficial to implement a similar fix there? I don't know if the 
> fix is as straightforward as in clang-tidy or if it will have to be done on a 
> per-check basis.

Speaking as just a user of the compiler and not a contributor (I tend to stick 
in Clang-Tidy land), yes, a fix would be great. Especially if you're getting 
unactionable warnings coming from STM32 libraries - it's a platform I have to 
develop for too. Not sure where in Clang you would make the change. At a 
cursory glance, the commits in `Clang/` I saw to do with stopping system header 
warnings from bubbling up were done in the individual warnings, not in common 
code. Best to check with someone who works in that area.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

In D116378#3226242 , @salman-javed-nz 
wrote:

> This resolves the long-standing gripe I have with clang-tidy raising warnings 
> about GoogleTest macros expanded in my code. Good work.
> Do you think we can close issues #44873 
> , #43325 
> , and #31587 
>  now that we have this fix?

Thanks! It has been bugging me for long time as well :)

It seems to me those issues could be closed, yes.

By the way, the similar problem exists in Clang compiler. I have written in 
cfe-dev , 
Discourse 

 and submitted an issue  
without any feedback so far (I understand it's been vacation). Do you think it 
would be beneficial to implement a similar fix there? I don't know if the fix 
is as straightforward as in clang-tidy or if it will have to be done on a 
per-check basis.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added a comment.

This resolves the long-standing gripe I have with clang-tidy raising warnings 
about GoogleTest macros expanded in my code. Good work.
Do you think we can close issues #44873 
, #43325 
, and #31587 
 now that we have this fix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez 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 rG670de10f9dea: Disable clang-tidy warnings from system macros 
(authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116378

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -9,6 +9,8 @@
 //   file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-NO-SYSTEM-HEADERS %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
@@ -71,3 +73,8 @@
 // CHECK4-NOT: Suppressed {{.*}} warnings
 // CHECK4-NOT: Use -header-filter=.* {{.*}}
 // CHECK4-QUIET-NOT: Suppressed
+
+int x = 123;
+auto x_ptr = TO_FLOAT_PTR(&x);
+// CHECK5: :[[@LINE-1]]:14: warning: do not use C-style cast to convert between unrelated types
+// CHECK5-NO-SYSTEM-HEADERS-NOT: :[[@LINE-2]]:14: warning: do not use C-style cast to convert between unrelated types
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
===
--- clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
@@ -1 +1,3 @@
 class A0 { A0(int); };
+
+#define TO_FLOAT_PTR(x) ((float *)(x))
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,9 @@
 Improvements to clang-tidy
 --
 
+- Ignore warnings from macros defined in system headers, if not using the
+  `-system-headers` flag.
+
 - Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
@@ -151,7 +154,7 @@
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   ` for C++17 deduction guides.
-  
+
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   ` when creating an exception object
   using placement new
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
@@ -28,6 +28,8 @@
 return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 };
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
@@ -11,6 +11,9 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/Token.h"
 
 using namespace clang::ast_matchers;
 
@@ -57,6 +60,10 @@
 // clang-format on
 };
 
+static constexpr 

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added a comment.

Thanks for the review! I've fixed the nit, reverted unrelated changes to 
backticks and moved the class to the existing anonymous namespace. Since those 
are NFC changes I'll push now, if you don't agree let me know and I'll revert 
right away :)




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:32-33
+  Check->diag(MacroNameTok.getLocation(),
+  "do not use va_arg to define c-style vararg functions; "
+  "use variadic templates instead");
+}

aaron.ballman wrote:
> Rather than duplicate the diagnostic text, I think we should hoist the text 
> into a constant string that's used in both places.
> 
> That makes it easier to fix the diagnostic (in a follow up) to properly quote 
> `va_arg` and convert the grammar to singular form instead of plural (e.g, `do 
> not use 'va_arg' to define a C-style vararg function; use a variadic template 
> instead`).
Of course!


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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 397958.
carlosgalvezp added a comment.

- Refactor duplicated warning message into a common variable.
- Remove double backticks change, TBH I get conflicting review comments about 
this and can't find any docs supporting either, so I'll just keep it consistent.
- Move extra class to the existing anonymous namespace instead of creating a 
new one.


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

https://reviews.llvm.org/D116378

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -9,6 +9,8 @@
 //   file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-NO-SYSTEM-HEADERS %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
@@ -71,3 +73,8 @@
 // CHECK4-NOT: Suppressed {{.*}} warnings
 // CHECK4-NOT: Use -header-filter=.* {{.*}}
 // CHECK4-QUIET-NOT: Suppressed
+
+int x = 123;
+auto x_ptr = TO_FLOAT_PTR(&x);
+// CHECK5: :[[@LINE-1]]:14: warning: do not use C-style cast to convert between unrelated types
+// CHECK5-NO-SYSTEM-HEADERS-NOT: :[[@LINE-2]]:14: warning: do not use C-style cast to convert between unrelated types
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
===
--- clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
@@ -1 +1,3 @@
 class A0 { A0(int); };
+
+#define TO_FLOAT_PTR(x) ((float *)(x))
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,9 @@
 Improvements to clang-tidy
 --
 
+- Ignore warnings from macros defined in system headers, if not using the
+  `-system-headers` flag.
+
 - Added support for globbing in `NOLINT*` expressions, to simplify suppressing
   multiple warnings in the same line.
 
@@ -151,7 +154,7 @@
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   ` for C++17 deduction guides.
-  
+
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   ` when creating an exception object
   using placement new
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
@@ -28,6 +28,8 @@
 return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 };
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
@@ -11,6 +11,9 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
 #include "clang/Basic/TargetInfo.h"
+#include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/Tok

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[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.

LGTM aside from a minor nit, thanks! This was a good catch.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:32-33
+  Check->diag(MacroNameTok.getLocation(),
+  "do not use va_arg to define c-style vararg functions; "
+  "use variadic templates instead");
+}

Rather than duplicate the diagnostic text, I think we should hoist the text 
into a constant string that's used in both places.

That makes it easier to fix the diagnostic (in a follow up) to properly quote 
`va_arg` and convert the grammar to singular form instead of plural (e.g, `do 
not use 'va_arg' to define a C-style vararg function; use a variadic template 
instead`).


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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:23-40
+namespace {
+class VaArgPPCallbacks : public PPCallbacks {
+public:
+  VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+SourceRange Range, const MacroArgs *Args) override {

carlosgalvezp wrote:
> salman-javed-nz wrote:
> > Should this be in another patch?
> > 
> > Line 722 in ClangTidyDiagnosticConsumer.cpp makes it so that clang-tidy 
> > filters warnings from system macros. This would benefit all checks.
> > 
> > This VaArgPPCallBack change here only benefits 
> > `cppcoreguidelines-pro-type-vararg`.
> The change to `ClangTidyDiagnosticConsumer.cpp` broke this check, because it 
> was checking for the use of `vararg` indirectly, checking for the use of 
> `__builtin_vararg`, which was expanded from the system macro `vararg`. This 
> patch updates the code so it checks directly what the rule is about "do not 
> use `vararg`".  I should perhaps have been more clear about that in the 
> commit message :) 
Messed up names above, I meant `va_arg` and `__builtin_va_arg`.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:46-78
+"__builtin_isgreater",
+"__builtin_isgreaterequal",
 "__builtin_isless",
-"__builtin_islessequal", 
-"__builtin_islessgreater", 
+"__builtin_islessequal",
+"__builtin_islessgreater",
 "__builtin_isunordered",
+"__builtin_fpclassify",

carlosgalvezp wrote:
> salman-javed-nz wrote:
> > Formatting changed for code not directly involved in this patch. Should be 
> > moved to a separate NFC commit that you don't have to run by us.
> Yes, it's a bit unfortunate, this was an automatic change on save from my 
> editor. Is it really worth it to revert this change though? I will need to 
> disable this feature on my IDE, revert each line manually by adding a space, 
> push, then re-enable the feature. Sounds like a lot of time spent on 
> reverting a change that anyway improves the state of the code.
Fixed.


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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp updated this revision to Diff 397859.
carlosgalvezp added a comment.

Revert formatting, clarify necessary changes to existing check in the commit 
message.


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

https://reviews.llvm.org/D116378

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
  clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
@@ -9,6 +9,8 @@
 //   file-filter\header*.h due to code order between '/' and '\\'.
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -system-headers -quiet %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK4-QUIET %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' -system-headers %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5 %s
+// RUN: clang-tidy -checks='-*,cppcoreguidelines-pro-type-cstyle-cast' -header-filter='.*' %s -- -I %S/Inputs/file-filter/system/.. -isystem %S/Inputs/file-filter/system 2>&1 | FileCheck --check-prefix=CHECK5-NO-SYSTEM-HEADERS %s
 
 #include "header1.h"
 // CHECK-NOT: warning:
@@ -71,3 +73,8 @@
 // CHECK4-NOT: Suppressed {{.*}} warnings
 // CHECK4-NOT: Use -header-filter=.* {{.*}}
 // CHECK4-QUIET-NOT: Suppressed
+
+int x = 123;
+auto x_ptr = TO_FLOAT_PTR(&x);
+// CHECK5: :[[@LINE-1]]:14: warning: do not use C-style cast to convert between unrelated types
+// CHECK5-NO-SYSTEM-HEADERS-NOT: :[[@LINE-2]]:14: warning: do not use C-style cast to convert between unrelated types
Index: clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
===
--- clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
+++ clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
@@ -1 +1,3 @@
 class A0 { A0(int); };
+
+#define TO_FLOAT_PTR(x) ((float *)(x))
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,10 +67,13 @@
 Improvements to clang-tidy
 --
 
-- Added support for globbing in `NOLINT*` expressions, to simplify suppressing
+- Ignore warnings from macros defined in system headers, if not using the
+  ``-system-headers`` flag.
+
+- Added support for globbing in ``NOLINT*`` expressions, to simplify suppressing
   multiple warnings in the same line.
 
-- Added support for `NOLINTBEGIN` ... `NOLINTEND` comments to suppress
+- Added support for ``NOLINTBEGIN`` ... ``NOLINTEND`` comments to suppress
   Clang-Tidy warnings over multiple lines.
 
 - Generalized the `modernize-use-default-member-init` check to handle non-default
@@ -151,7 +154,7 @@
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   ` for C++17 deduction guides.
-  
+
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   ` when creating an exception object
   using placement new
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
@@ -28,6 +28,8 @@
 return LangOpts.CPlusPlus;
   }
   void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
+   Preprocessor *ModuleExpanderPP) override;
   void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
 };
 
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
@@ -11,12 +11,33 @@
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMat

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:23-40
+namespace {
+class VaArgPPCallbacks : public PPCallbacks {
+public:
+  VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+SourceRange Range, const MacroArgs *Args) override {

salman-javed-nz wrote:
> Should this be in another patch?
> 
> Line 722 in ClangTidyDiagnosticConsumer.cpp makes it so that clang-tidy 
> filters warnings from system macros. This would benefit all checks.
> 
> This VaArgPPCallBack change here only benefits 
> `cppcoreguidelines-pro-type-vararg`.
The change to `ClangTidyDiagnosticConsumer.cpp` broke this check, because it 
was checking for the use of `vararg` indirectly, checking for the use of 
`__builtin_vararg`, which was expanded from the system macro `vararg`. This 
patch updates the code so it checks directly what the rule is about "do not use 
`vararg`".  I should perhaps have been more clear about that in the commit 
message :) 



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:46-78
+"__builtin_isgreater",
+"__builtin_isgreaterequal",
 "__builtin_isless",
-"__builtin_islessequal", 
-"__builtin_islessgreater", 
+"__builtin_islessequal",
+"__builtin_islessgreater",
 "__builtin_isunordered",
+"__builtin_fpclassify",

salman-javed-nz wrote:
> Formatting changed for code not directly involved in this patch. Should be 
> moved to a separate NFC commit that you don't have to run by us.
Yes, it's a bit unfortunate, this was an automatic change on save from my 
editor. Is it really worth it to revert this change though? I will need to 
disable this feature on my IDE, revert each line manually by adding a space, 
push, then re-enable the feature. Sounds like a lot of time spent on reverting 
a change that anyway improves the state of the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116378

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

[PATCH] D116378: [clang-tidy] Disable clang-tidy warnings from system macros

[Salman Javed via Phabricator via cfe-commits]

salman-javed-nz added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:23-40
+namespace {
+class VaArgPPCallbacks : public PPCallbacks {
+public:
+  VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+SourceRange Range, const MacroArgs *Args) override {

Should this be in another patch?

Line 722 in ClangTidyDiagnosticConsumer.cpp makes it so that clang-tidy filters 
warnings from system macros. This would benefit all checks.

This VaArgPPCallBack change here only benefits 
`cppcoreguidelines-pro-type-vararg`.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp:46-78
+"__builtin_isgreater",
+"__builtin_isgreaterequal",
 "__builtin_isless",
-"__builtin_islessequal", 
-"__builtin_islessgreater", 
+"__builtin_islessequal",
+"__builtin_islessgreater",
 "__builtin_isunordered",
+"__builtin_fpclassify",

Formatting changed for code not directly involved in this patch. Should be 
moved to a separate NFC commit that you don't have to run by us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116378

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