[libcxx-commits] [PATCH] D117174: [libc++][test] Run clang-tidy during CI

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

Looks acceptable to me mod this last round of comments. It should be rebased after D119295 <https://reviews.llvm.org/D119295> and re-uploaded, but then (if that works, which is why I'm still "requesting changes" on this round) I don't see a really compelling reason //not// to land it...



================
Comment at: libcxx/test/libcxx/clang_tidy.sh.cpp:9
+
+// UNSUPPORTED: !has-clang-tidy
+// XFAIL: modules-build
----------------
I think this should just be `REQUIRES: has-clang-tidy`


================
Comment at: libcxx/test/libcxx/clang_tidy.sh.cpp:12
+// RUN: clang-tidy %s --warnings-as-errors=* -header-filter=.* -- -Wno-unknown-warning-option %{compile_flags}
+// -------------------------------------------------------------- ^ ignore GCC warnings unknown to clang
+
----------------
The number of `-`s on this line will bit-rot quickly. Consider replacing with a full-sentence comment like
```
// -Wno-unknown-warning-option tells clang-tidy to ignore "#pragma GCC diagnostic" options it doesn't recognize.
```
Although, that shouldn't be a problem now that D119295 has landed. So can you remove `-Wno-unknown-warning-option` at this point? 


================
Comment at: libcxx/test/libcxx/clang_tidy.sh.cpp:9-12
+// UNSUPPORTED: !has-clang-tidy
+// XFAIL: modules
+// RUN: clang-tidy %s --warnings-as-errors=* -header-filter=.* -- -Wno-unknown-warning-option %{compile_flags}
+// -------------------------------------------------------------- ^ ignore GCC warnings unknown to clang
----------------
philnik wrote:
> Quuxplusone wrote:
> > > `XFAIL: modules`
> > What's the problem with modules? (Preferably in the form of an English explanation //plus// a link to the failing buildkite.)
> https://buildkite.com/llvm-project/libcxx-ci/builds/8619
> It complains with `use of private header from outside its module` and `function-like macro '__has_keyword' is not defined`. Also a few other macros.
Hmm, looks like clang-tidy doesn't play well with modules, huh? OK.

This reminds me that clang-tidy doesn't look inside `#ifdef`s either. So we're losing test coverage for //include ordering// in any detail header that's not included on a platform we test in buildkite. I don't //like// that, but I'm willing to ignore it for now and just revisit once it becomes a problem.  (The obvious way to do that is for me to just keep running the original `lint-headers.sh.py` locally every month until it fails, and then complain about it at that point. But I'm almost certainly too lazy for that.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117174