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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/test/libcxx/lint/lint_headers.sh.py:1
-# RUN: %{python} %s
-
----------------
Quuxplusone wrote:
> philnik wrote:
> > Quuxplusone wrote:
> > > Please don't delete `lint_headers.sh.py`. It does things that clang-tidy can't, such as check for the existence of `#  pragma GCC system_header` and (soon) check for the existence of `#include <__config>`.
> > > We can discuss whether the 12 lines concerned with include-sorting are now permanently obsoleted (because we trust clang-tidy to check include-sorting forever more), but FYI //personally// I don't trust clang-tidy and //personally// I don't think 12 lines is too much to pay for peace of mind. Again, if clang-tidy ever disagreed with this script, clang-tidy would be wrong by definition.
> > clang-tidy does //exactly// the same thing as this script. I don't think it's much of an effort to re-create the parts of this script that do the include sorting, in case we want something else. One advantage of clang-tidy is that it is shown in your IDE. If you don't use an IDE OK, but for the people that do use and IDE (with clang-tidy integration), clang-tidy will warn on include-order in the IDE and you don't have to run the tests to check that. That is simply not //possible// to do with a script portably.
> > 
> > Why don't you trust clang-tidy? Do you think it will break some code? Why would clang-tidy be wrong by definition?
> > I understand that you want to keep this script. It does things clang-tidy is not designed to do. But why don't you want clang-tidy to check for includes?
> > clang-tidy does exactly the same thing as this script.
> 
> That's untrue.
> 
> > I don't think it's much of an effort to re-create the parts of this script that do the include sorting
> 
> Right. It's like 12 lines. Just leave them in place. If we later find out there's a problem with them, we can fix them. I'm not objecting to your //adding// the dependency on clang-tidy; I'm objecting to //removing// this lint script (which checks for include ordering in 12 lines of code, and checks for `#pragma GCC system_header` in 4 more lines, and will soon check for `#include <__config>` in another 4.) Go ahead and add clang-tidy to your workflow. Just please don't //remove// this script, that's all.
> 
> > Why would clang-tidy be wrong by definition?
> 
> //If clang-tidy ever disagrees with this script//, then clang-tidy is wrong by definition, because this script (by definition) will always express exactly the invariants that libc++ wants to check for. If clang-tidy (hypothetically) ever failed to catch something that this script caught, that would be a false negative in clang-tidy (and maybe we'd want to file a bug upstream for it). If clang-tidy (hypothetically) ever complained about something that this script considered OK, that would be a false positive in clang-tidy (and maybe we'd want to file a bug upstream for it). I'm not claiming that clang-tidy is //necessarily// wrong; I'm just saying that libc++'s own custom invariant-checking script is correct by definition, so //if// clang-tidy ever disagreed with this script then obviously (by definition) clang-tidy would be producing the "wrong" output.
(whenever I say 'script' I mean the include-order check)
My problem with leaving them both in is that it would be //possible// that they contradict each other, and I don't see a good reason to have this possibility. You could just as well define that clang-tidy is always right and that would make the script wrong. If we remove the script we have eliminated the possibility of contradiction while having no downside AFAICT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117174