[libcxx-commits] [PATCH] D143333: [libc++] Run clang-tidy in all configurations that are run in the Docker container

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:318
           CXX: "clang++-${LLVM_HEAD_VERSION}"
+          ENABLE_CLANG_TIDY: "On"
       agents:
----------------
philnik wrote:
> Mordante wrote:
> > philnik wrote:
> > > ldionne wrote:
> > > > philnik wrote:
> > > > > Mordante wrote:
> > > > > > The clang-tidy tests don't seem to be very cheap. I expect them to become more expensive in the future, since we probably want to add more tests. (I have some ideas on my own todo list.) So I wonder, does it have real benefit to enable them in all configurations? Especially this one. If it does then I think it would be good write the rationale in the commit message.
> > > > > Most configurations have some code that's specific to it.
> > > > > 
> > > > > For me running clang-tidy takes ~40s. While that's not quick, it's also not exactly unbearable. We have other tests that take a lot longer. By far the slowest check is `readability-identifier-naming`, removing it brings the time down to ~8s. So I don't expect running clang-tidy to take that much longer in the future, since `nasty_macros.compile.pass.cpp` takes ~2s which is effectively just running the front-end. Since we run clang-tidy two times, all the checks except `readability-identifier-naming` take ~4s.
> > > > At the risk of asking a dumb question, what's the benefit of running the checks in all configurations?
> > > We cover code that is specific to the configuration. For example, the check that makes sure we use _Uglified attributes doesn't catch uses of `__attribute__((trivial_abi))` currently. I only changed it because I ran clang-tidy with the unstable ABI locally. While I'm not aware of any TSan-specific code, there is code specific to ASan and UBSan.
> > I see it takes about 80 s in the CI, not the cheapest, but not the most expensive either. I see the usefulness of different configurations, but I think some are not really useful.
> > 
> > AFAIK in GCC we disabled clang-tidy since there are conflicting compiler warnings, we also have very few work-arounds for older Clang versions, so there I too expect less warnings.
> > 
> > So I don't mind to enable it when we really expect different code paths to be validated, but we shouldn't blanket enable it.
> I disagree. We wouldn't even have this discussion if we were able to just detect clang-tidy properly. We do this for no other slow test we have, so IMO we shouldn't treat it any different. This will just bite us in the future otherwise. I also don't think that 80s on a single thread makes much of a difference at large. This will increase the runtime by maybe two or three seconds per bot, which is a lot less than the variance we have between runs.
Yeah, I agree, let's do it. Testing systematically is the simplest and the best approach, we should be addressing CI times as a general problem but not by disabling specific tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143333