add /_virtual_includes/ as -isystem

[adrianimboden]

I have a maybe controversial addon here (probably wen can make it configurable somehow), but I think this might help other users as well.

Background: our clang-tidy config (probably others as well) have this configuration inside:

HeaderFilterRegex: '.*'

That way we include our own code that only lives in the header files in the clang-tidy check.

Actually, the config is a little bit simplified. It looks like this in reality:

HeaderFilterRegex: '^(([^e].*$)|(e([^x].*$))|(ex([^t].*$))|(ext([^e].*$))|(exte([^r].*$))|(exter([^n].*$))|(extern([^a].*$))|(externa([^l].*$))|(external([^/].*$)))'

The reason for this overly complex regex is:

• clang-tidy sadly only supports header inclusion rules, but not exclusions (maybe I'm looking into that code to change that in the future)
• clang-tidy regexes don't support lookahead/lookback or other more advanced regex features, so writing the exclusions is very complex, as can be seen in the above example (it matches everything except strings starting with external/)

Now this worked for most things in the codebase, until I saw a special case:

• when bazel uses include_prefix or strip_include_prefix, then paths like path/to/$PACKAGE/_virtual_includes/$TARGET are generated and put on the include path
• this happens for example in google benchmark that we also use: https://github.com/google/benchmark/blob/main/BUILD.bazel#L46

The best solution would be to change clang-tidy of course. This change here would probably help other users as well as it excludes those paths by putting them into the system includes.

The patch should not generate problems because:

• I think that users do not include the system paths very often (that would mean they use system libraries, which in bazel should be rare)
• _virtual_includes should not be a common folder name that a user chooses manually
• when HeaderFilterRegex is unset, this change has no effect as everything is excluded anyway

[erenon]

Thanks for the PR, and the detailed description. Sorry for the late reply.

I think that users do not include the system paths very often (that would mean they use system libraries, which in bazel should be rare)

Unfortunately, C++ standard doesn't make a difference between different inclusion styles. There's at least one big C++ shop, that (for historical reasons) uses <> includes for everything. (It is a valid decision imo: the difference between a system library and a user library is often very thin or ambiguous) Therefore, as it stands, this PR is a breaking change, and I would like to avoid merging it. Perhaps make it opt-it?