Author: Carlos Galvez
Date: 2022-01-06T20:27:28Z
New Revision: 670de10f9deaa83f4d1db6e793c74cbfd18c65c1

URL: 
https://github.com/llvm/llvm-project/commit/670de10f9deaa83f4d1db6e793c74cbfd18c65c1
DIFF: 
https://github.com/llvm/llvm-project/commit/670de10f9deaa83f4d1db6e793c74cbfd18c65c1.diff

LOG: Disable clang-tidy warnings from system macros

Currently, it's inconsistent that warnings are disabled if they
come from system headers, unless they come from macros.
Typically a user cannot act upon these warnings coming from
system macros, so clang-tidy should ignore them unless the
user specifically requests warnings from system headers
via the corresponding configuration.

This change broke the ProTypeVarargCheck check, because it
was checking for the usage of va_arg indirectly, expanding it
(it's a system macro) to detect the usage of __builtin_va_arg.
The check has been fixed by checking directly what the rule
is about: "do not use va_arg", by adding a PP callback that
checks if any macro with name "va_arg" is expanded. The old
AST matcher is still kept for compatibility with Windows.

Add unit test that ensures warnings from macros are disabled
when not using the -system-headers flag. Document the change
in the Release Notes.

Differential Revision: https://reviews.llvm.org/D116378

Added: 
    

Modified: 
    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

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp 
b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 6c5054fdca28e..a76fe16effb6a 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -719,7 +719,7 @@ void 
ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
   }
 
   if (!*Context.getOptions().SystemHeaders &&
-      Sources.isInSystemHeader(Location))
+      (Sources.isInSystemHeader(Location) || 
Sources.isInSystemMacro(Location)))
     return;
 
   // FIXME: We start with a conservative approach here, but the actual type of

diff  --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
index b26cd59fd672a..83924f977d1a6 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
+++ b/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 @@ static constexpr StringRef AllowedVariadics[] = {
     // clang-format on
 };
 
+static constexpr StringRef VaArgWarningMessage =
+    "do not use va_arg to define c-style vararg functions; "
+    "use variadic templates instead";
+
 namespace {
 AST_MATCHER(QualType, isVAList) {
   ASTContext &Context = Finder->getASTContext();
@@ -106,6 +113,21 @@ AST_MATCHER_P(AdjustedType, hasOriginalType,
               ast_matchers::internal::Matcher<QualType>, InnerType) {
   return InnerType.matches(Node.getOriginalType(), Finder, Builder);
 }
+
+class VaArgPPCallbacks : public PPCallbacks {
+public:
+  VaArgPPCallbacks(ProTypeVarargCheck *Check) : Check(Check) {}
+
+  void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
+                    SourceRange Range, const MacroArgs *Args) override {
+    if (MacroNameTok.getIdentifierInfo()->getName() == "va_arg") {
+      Check->diag(MacroNameTok.getLocation(), VaArgWarningMessage);
+    }
+  }
+
+private:
+  ProTypeVarargCheck *Check;
+};
 } // namespace
 
 void ProTypeVarargCheck::registerMatchers(MatchFinder *Finder) {
@@ -125,6 +147,12 @@ void ProTypeVarargCheck::registerMatchers(MatchFinder 
*Finder) {
       this);
 }
 
+void ProTypeVarargCheck::registerPPCallbacks(const SourceManager &SM,
+                                             Preprocessor *PP,
+                                             Preprocessor *ModuleExpanderPP) {
+  PP->addPPCallbacks(std::make_unique<VaArgPPCallbacks>(this));
+}
+
 static bool hasSingleVariadicArgumentWithValue(const CallExpr *C, uint64_t I) {
   const auto *FDecl = dyn_cast<FunctionDecl>(C->getCalleeDecl());
   if (!FDecl)
@@ -153,9 +181,7 @@ void ProTypeVarargCheck::check(const 
MatchFinder::MatchResult &Result) {
   }
 
   if (const auto *Matched = Result.Nodes.getNodeAs<Expr>("va_use")) {
-    diag(Matched->getExprLoc(),
-         "do not use va_arg to define c-style vararg functions; "
-         "use variadic templates instead");
+    diag(Matched->getExprLoc(), VaArgWarningMessage);
   }
 
   if (const auto *Matched = Result.Nodes.getNodeAs<VarDecl>("va_list")) {

diff  --git 
a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h 
b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
index 06de06e3e8361..08b879abf130a 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.h
@@ -28,6 +28,8 @@ class ProTypeVarargCheck : public ClangTidyCheck {
     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;
 };
 

diff  --git a/clang-tools-extra/docs/ReleaseNotes.rst 
b/clang-tools-extra/docs/ReleaseNotes.rst
index 5d330d1d2e3cf..03a52fa7c7aec 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -67,6 +67,9 @@ The improvements are...
 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 @@ Changes in existing checks
 
 - Fixed a false positive in :doc:`fuchsia-trailing-return
   <clang-tidy/checks/fuchsia-trailing-return>` for C++17 deduction guides.
-  
+
 - Fixed a false positive in :doc:`bugprone-throw-keyword-missing
   <clang-tidy/checks/bugprone-throw-keyword-missing>` when creating an 
exception object
   using placement new

diff  --git 
a/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
 
b/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
index 98482c4277dda..6e338c85f394e 100644
--- 
a/clang-tools-extra/test/clang-tidy/infrastructure/Inputs/file-filter/system/system-header.h
+++ 
b/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))

diff  --git a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp 
b/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
index 9ee5cad979f6a..a7498723de2bf 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/file-filter.cpp
+++ b/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 @@ class A { A(int); };
 // 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


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

Reply via email to