DmitryPolukhin created this revision.
DmitryPolukhin added reviewers: alexfh, njames93, thakis.
DmitryPolukhin added projects: clang, clang-tools-extra.
Herald added subscribers: kbarton, xazax.hun, nemanjai.
DmitryPolukhin requested review of this revision.
This diff is an attempt to workaround a common problem with clang-tidy
deployment.
There are headers that might not be compatible with your project style guide
but you
have to use macro from that headers so clang-tidy could report lots of style
issues
that you cannot fix or is some cases even annotate with NOLINT. Solution
proposed
in this diff is to avoid reporting diagnostics in macro expansion of the macro
defined
in headers that doesn't match the desired `--header-filter`.
Just one real life example of this issue from a very popular gflags library.
This library requires using macro in use code like this:
DEFINE_bool(some_bool_flag,
"the default value should go here, not the description",
false);
But the macro has outdated version of compiler assert that uses C-style arrays
<https://github.com/gflags/gflags/blob/2e227c3daae2ea8899f49858a23f3d318ea39b57/src/gflags.h.in#L528>.
Therefore use code becomes incompatible with clang-tidy check
`modernize-avoid-c-arrays`.
Another example of problematic is googletest/googlemock with lots of macro that
you cannot avoid.
This diff implements new behavior by default but it might be also possible to
enable it behind some configuration flag.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D90835
Files:
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/clang-tidy/cppcoreguidelines/ProTypeVarargCheck.cpp
clang-tools-extra/test/clang-tidy/infrastructure/macros.cpp
clang-tools-extra/test/clang-tidy/infrastructure/macros.h
clang-tools-extra/test/clang-tidy/infrastructure/macros_filter.h
Index: clang-tools-extra/test/clang-tidy/infrastructure/macros_filter.h
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/macros_filter.h
@@ -0,0 +1,3 @@
+#define HEADER_FILTER_MACRO_DIAG_IN_ARG(a) a
+#define HEADER_FILTER_MACRO_DIAG_IN_BODY int var_B1[10]
+#define HEADER_FILTER_MACRO_DIAG_IN_BODY2 int var_B2[10]
Index: clang-tools-extra/test/clang-tidy/infrastructure/macros.h
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/macros.h
@@ -0,0 +1,9 @@
+#define HEADER_MACRO_DIAG_IN_ARG(a) a
+#define HEADER_MACRO_DIAG_IN_BODY int var_C1[10]
+#define HEADER_MACRO_DIAG_IN_BODY2 int var_C2[10]
+
+#define DEFINE_bool(name, val) \
+ namespace fLB { \
+ typedef char FLAG_##name##_value_is_not_a_bool[(sizeof(val) != sizeof(bool)) ? -1 : 1]; \
+ } \
+ bool FLAG_##name = val
Index: clang-tools-extra/test/clang-tidy/infrastructure/macros.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/infrastructure/macros.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/macros.cpp
@@ -1,7 +1,41 @@
-// RUN: clang-tidy -checks='-*,google-explicit-constructor' %s -- | FileCheck %s
+// RUN: clang-tidy -checks='-*,google-explicit-constructor,modernize-avoid-c-arrays' --header-filter='macros_filter.h' %s -- | FileCheck %s
+
+#include "macros.h"
+#include "macros_filter.h"
#define Q(name) class name { name(int i); }
Q(A);
// CHECK: :[[@LINE-1]]:3: warning: single-argument constructors must be marked explicit
-// CHECK: :3:30: note: expanded from macro 'Q'
+// CHECK: :[[@LINE-4]]:30: note: expanded from macro 'Q'
+
+#define MAIN_MACRO_DIAG_IN_ARG(a) a
+MAIN_MACRO_DIAG_IN_ARG(int var_A[10]);
+// CHECK: :[[@LINE-1]]:24: warning: do not declare C-style arrays, use std::array<> instead
+
+#define MAIN_MACRO_DIAG_IN_BODY int var_A1[10]
+MAIN_MACRO_DIAG_IN_BODY;
+// CHECK: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+// CHECK: :[[@LINE-3]]:33: note: expanded from macro 'MAIN_MACRO_DIAG_IN_BODY'
+
+HEADER_FILTER_MACRO_DIAG_IN_ARG(int var_B[10]);
+// CHECK: :[[@LINE-1]]:33: warning: do not declare C-style arrays, use std::array<> instead
+
+HEADER_FILTER_MACRO_DIAG_IN_BODY;
+// CHECK: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+// CHECK: note: expanded from macro 'HEADER_FILTER_MACRO_DIAG_IN_BODY'
+
+#define MAIN_MACRO_WRAPPER HEADER_FILTER_MACRO_DIAG_IN_BODY2
+MAIN_MACRO_WRAPPER;
+// CHECK: :[[@LINE-1]]:1: warning: do not declare C-style arrays, use std::array<> instead
+// CHECK: note: expanded from macro 'MAIN_MACRO_WRAPPER'
+// CHECK: note: expanded from macro 'HEADER_FILTER_MACRO_DIAG_IN_BODY2'
+
+HEADER_MACRO_DIAG_IN_ARG(int var_C[10]);
+HEADER_MACRO_DIAG_IN_BODY;
+
+#define MAIN_MACRO_WRAPPER2 HEADER_MACRO_DIAG_IN_BODY2
+MAIN_MACRO_WRAPPER2;
+
+DEFINE_bool(test, false);
+// CHECK-NOT: warning:
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
@@ -115,7 +115,10 @@
}
if (const auto *Matched = Result.Nodes.getNodeAs<Expr>("va_use")) {
- diag(Matched->getExprLoc(),
+ // va_arg is a macro defined in system header but diags from such macro
+ // are not always reported so use expansion location instead to always
+ // report
+ diag(Result.SourceManager->getExpansionLoc(Matched->getExprLoc()),
"do not use va_arg to define c-style vararg functions; "
"use variadic templates instead");
}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -177,6 +177,10 @@
DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID)));
}
+ /// Returns the \c HeaderFilter constructed for the options set in the
+ /// context.
+ llvm::Regex *getHeaderFilter() const;
+
private:
// Writes to Stats.
friend class ClangTidyDiagnosticConsumer;
@@ -189,6 +193,8 @@
class CachedGlobList;
std::unique_ptr<CachedGlobList> CheckFilter;
std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
+ // Cache for compiled regex with HeaderFilter.
+ mutable std::unique_ptr<llvm::Regex> HeaderFilter;
LangOptions LangOpts;
@@ -243,10 +249,6 @@
void removeIncompatibleErrors();
void removeDuplicatedDiagnosticsOfAliasCheckers();
- /// Returns the \c HeaderFilter constructed for the options set in the
- /// context.
- llvm::Regex *getHeaderFilter();
-
/// Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
/// according to the diagnostic \p Location.
void checkFilters(SourceLocation Location, const SourceManager &Sources);
@@ -258,7 +260,6 @@
DiagnosticsEngine *ExternalDiagEngine;
bool RemoveIncompatibleErrors;
std::vector<ClangTidyError> Errors;
- std::unique_ptr<llvm::Regex> HeaderFilter;
bool LastErrorRelatesToUserCode;
bool LastErrorPassesLineFilter;
bool LastErrorWasIgnored;
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===================================================================
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -245,6 +245,13 @@
return "";
}
+llvm::Regex *ClangTidyContext::getHeaderFilter() const {
+ if (!HeaderFilter)
+ HeaderFilter =
+ std::make_unique<llvm::Regex>(*getOptions().HeaderFilterRegex);
+ return HeaderFilter.get();
+}
+
ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
bool RemoveIncompatibleErrors)
@@ -331,6 +338,41 @@
return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
}
+static bool IsErrorFromMacroInIgnoredHeader(const SourceManager &SM,
+ SourceLocation Loc,
+ const ClangTidyContext &Context) {
+ SourceLocation SpellingLoc = SM.getImmediateSpellingLoc(Loc);
+ // Don't skip macro from the main file.
+ if (SM.isInMainFile(SpellingLoc))
+ return false;
+
+ // Skip macro from system headers.
+ if (!*Context.getOptions().SystemHeaders && SM.isInSystemHeader(SpellingLoc))
+ return true;
+
+ FileID FID = SM.getDecomposedExpansionLoc(SpellingLoc).first;
+ const FileEntry *File = SM.getFileEntryForID(FID);
+
+ // -DMACRO definitions on the command line have locations in a virtual buffer
+ // that doesn't have a FileEntry. Don't skip these.
+ if (!File) {
+ return false;
+ }
+
+ StringRef FileName(File->getName());
+ if (const FileEntry *MainFile = SM.getFileEntryForID(SM.getMainFileID())) {
+ if (MainFile->getName() == FileName) {
+ // This check should be equvalent to isInMainFile but when clangd loads
+ // preamble isInMainFile gives wrong result for macro spelling location.
+ // It looks like the location comes from file included into itself at
+ // position 1:1. This check fixes ClangdTests/DiagnosticsTest.ClangTidy
+ // TODO: find out what clangd does wrong with location
+ return false;
+ }
+ }
+ return !Context.getHeaderFilter()->match(FileName);
+}
+
static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
SourceLocation Loc, unsigned DiagID,
const ClangTidyContext &Context,
@@ -340,6 +382,8 @@
return true;
if (!Loc.isMacroID())
return false;
+ if (IsErrorFromMacroInIgnoredHeader(SM, Loc, Context))
+ return true;
Loc = SM.getImmediateExpansionRange(Loc).getBegin();
}
return false;
@@ -550,20 +594,13 @@
StringRef FileName(File->getName());
LastErrorRelatesToUserCode = LastErrorRelatesToUserCode ||
Sources.isInMainFile(Location) ||
- getHeaderFilter()->match(FileName);
+ Context.getHeaderFilter()->match(FileName);
unsigned LineNumber = Sources.getExpansionLineNumber(Location);
LastErrorPassesLineFilter =
LastErrorPassesLineFilter || passesLineFilter(FileName, LineNumber);
}
-llvm::Regex *ClangTidyDiagnosticConsumer::getHeaderFilter() {
- if (!HeaderFilter)
- HeaderFilter =
- std::make_unique<llvm::Regex>(*Context.getOptions().HeaderFilterRegex);
- return HeaderFilter.get();
-}
-
void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
// Each error is modelled as the set of intervals in which it applies
// replacements. To detect overlapping replacements, we use a sweep line
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
