[PATCH] D90835: [RFC][clang-tidy] Ignore diagnostics due to macro expansion from not-interested headers

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added a comment.

@njames93 thank you for the feedback!

Still waiting for more feedback and especially high level one about this feature in general. Does it look useful? Should it be behind a flag i.e. changing default behaviour is too dangerous?



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:249-251
+  if (!HeaderFilter)
+    HeaderFilter =
+        std::make_unique<llvm::Regex>(*getOptions().HeaderFilterRegex);
----------------
njames93 wrote:
> This should also check if the Optional has a value
This code was moved from `ClangTidyDiagnosticConsumer::getHeaderFilter` but you are right, it makes sense to add support for missing value.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:349-351
+  // Skip macro from system headers.
+  if (!*Context.getOptions().SystemHeaders && SM.isInSystemHeader(SpellingLoc))
+    return true;
----------------
njames93 wrote:
> This looks suspicious, in clang-tidy land `SystemHeaders` is always set, however outside clang-tidy it may not be.
> Perhaps `getValueOr(false)` should be used to prevent any asserts.
> 
> Also can a test be added to show this respecting the SystemHeaders setting.
Running this code on bigger codebase I found undesired side-effect that it is better to report issues on system macro like NULL such issues usually from external code, not from macro itself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90835