[PATCH] D149899: [clang-tidy] Support SystemHeaders in .clang-tidy

Fri May 5 04:12:37 PDT 2023

PiotrZSL added inline comments.

================
Comment at: clang-tools-extra/test/clang-tidy/infrastructure/system-headers.cpp:11
+// RUN: clang-tidy -checks='-*,google-explicit-constructor' -header-filter='.*' -config='SystemHeaders: false' %s -- -isystem %S/Inputs/system-headers 2>&1 | FileCheck -check-prefix=CHECK-NO-SYSTEM-HEADERS %s
+
+#include <system_header.h>
----------------
carlosgalvezp wrote:
> PiotrZSL wrote:
> > I missing 2 types of tests here:
> > - config value override, `-config='SystemHeaders: true' -system-headers=false`
> > - config file override, when one file has `false`, other `true`, but InheritConfig is used.
> > Both should work, but would be good to test them.
> Thanks for the input! Essentially I followed the same testing pattern as `UseColor`, which is also a global boolean option. 
> 
> Adding the first suggestion should be trivial in the current test case, but I have some doubts about the proposed second test. When I look at the `tests/clang-tidy/infrastructure/config-files.cpp` it only takes into account enabled checks, no other global options are tested such that one overrides the other. Therefore I wonder if there is a rationale for not having such level of testing, perhaps easier maintenance, given that it's already tested by the unit tests? In that case I would propose to not add such a test here, for consistency. WDYT?
I'm fine with that. I'm mainly concern about the first one, simply because I'm not sure about outcome, what will have higher priority, command line argument or config option.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149899