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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added a comment.

In D117174#3287755 <https://reviews.llvm.org/D117174#3287755>, @Quuxplusone wrote:

> In D117174#3286034 <https://reviews.llvm.org/D117174#3286034>, @philnik wrote:
>
>> @Quuxplusone I would remove the python lint. clang-tidy is less flexible, but objectively better. It also detects `#include`-order problems in `#ifdef`s and knows what `#include_next` is. Putting that into a python script is not really feasible in my opinion, but prove me wrong if you want to. I also wouldn't leave them side by side, because that could potentially mean they contradict each other.
>
> The point of my last comment was that //if// they contradict each other, then the Python script is correct by definition, but clang-tidy's misbehavior might be due to a bug that's out of our control.
>
> "It also detects `#include`-order problems in `#ifdef`s" — I'm not sure I understand this one. We certainly want to detect //every// consecutive run of `#include`s and alphabetize them, regardless of whether they're `#if 0`'ed (or even commented out). The Python script does this by definition (because it's not parsing C++, just looking for consecutive runs of `#include` lines). I don't know whether clang-tidy can handle `#if` or commented-out code (like our synopsis comments). I've been assuming it cannot. But anyway, "Don't make me think!" — rather than depend on reading clang-tidy's documentation to find out its behavior on `#if` and so on, it's strictly simpler to just say "here's a script that checks the invariant we care about." (I just checked again to make sure: the loop in `lint_headers.py` is only 12 lines long.)
>
> OTOH, for tasks where we //want// to skip commented-out synopsis code, such as making sure all our parameter names //outside// the synopsis are properly `_Uglified`, that would be a great fit for clang-tidy and a relatively worse fit for Python (although certainly still doable — thank goodness comments are easy to parse (because //another// libc++ invariant is that we don't use `R"(aw)"` strings)).

I thought there was a case where your script didn't check, but I can't reproduce it. I'd be happy to disable `llvm-include-order` if you add support for `#include_next` in `lint_headers.sh.py`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117174