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

Fri Jan 14 08:42:05 PST 2022

philnik added inline comments.

================
Comment at: libcxx/.clang-tidy:2
 InheritParentConfig: true
-Checks: '-readability-identifier-naming,-llvm-header-guard,-llvm-include-order,-misc-unconventional-assign-operator,-llvm-else-after-return'
----------------
ldionne wrote:
> Mordante wrote:
> > Quuxplusone wrote:
> > > One thing we should be wary of here is that by doing this, we're permitting the //clang-tidy devs// to break //libc++'s// unit tests.  Of course //Clang// and //GCC// devs already have this power, and we accept that.
> > > I'm guessing that if we discover clang-tidy suddenly diagnosing something we disagree with or don't have time to fix, then we can simply and quickly add another `-foo` to this list; or absolute worst case, simply UNSUPPORTED the test; so we should be quite capable of dealing with that kind of thing.
> > > I think this seems good (as long as the problem of installing clang-tidy on all relevant platforms/builders/Dockers gets solved; I have no special knowledge about that).
> > That can happen but I don't expect that to become an issue. In Docker we can pick a nightly build of clang-tidy-13 which should be stable. After clang-tidy-14 is released we can switch to that. In order to switch we need to update the Docker file. So the moment of the update is in our hands. (We already use this method to update to the different Clang and clang-format releases.)
> > 
> > I also wonder whether we need to enable it in all CI builds or only in a few. Especially since the clang-tidy run may not be cheap in resources. Once we have clang-tidy working we can see which options we like and dislike.
> > 
> > 
> Why are we removing `llvm-include-order`? What does this check do?
I'm actually adding `llvm-include-order` (note the `-` in front). It checks that the include order is correct (essentialy the same that @Quuxplusone 's include-order checker does).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117174