[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/clang_tidy.sh.cpp:12-13
+
+#pragma clang diagnostic push
+#pragma clang diagnostic ignored "-W#warnings"
+
----------------
Quuxplusone wrote:
> What does this pragma do, and what goes wrong if you omit it?
It ignores `#warnings` warnings. This test also checks `<ext/hash_map>` and `<ext/hash_set>`, which warn, because they have been replaced by `<unordered_map>` and `<unordered_set>`.


================
Comment at: libcxx/test/libcxx/lint/lint_headers.sh.py:1
-# RUN: %{python} %s
-
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117174