[PATCH] D83901: [clang] Disable a few formatting options for test/

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In D83901#2155127 <https://reviews.llvm.org/D83901#2155127>, @MyDeveloperDay wrote:

> clang-format -n warnings before this change in clang/test/Analysis/*.cpp  (clang-format -n *.cpp |& grep warning | wc -l)
>
> before = 6903 vs  after=6595, if it helps I'd say this looks good. A very brief review of changes seem to be mainly whitespace/extra lines and indentation  (44 in headers files vs 43)
>  (Note: There are 11 warnings after the first iteration, clang-format sometimes has to be run twice)
>
> The real proof is how many of the unit tests can be clang-formatted and still pass, but I completely approve of this effort, if the test directories were formatted this would give us a massive test suite of lots of different examples of c++ code to validate any clang-format changes.
>
> If having this in the clang-format file means users can use "Format on Save" options in their editors and not break tests then I think this is a good thing. As this will drive good behavior in all the other files rather than having to turn "Format on Save" off because we cannot maintain running tests.
>
> My 2 cents worth.


Thank you for your comment.

FWIW I think that the current approach with the lint bot needs to be changed for the following reasons:

1. `clang-format` lets me forget about formatting and for this I like it very much. This only works because running `clang-format` is almost always safe to run. ie: it doesn't change the meaning of the code. This breaks down with most tests in `test/` because the meaning of many test *is* changed by `clang-format`: ie: `clang-format` is very much *not* safe to run on a test.

2. Even after running `clang-format` on a test and fixing it manually, the bot will still complain in some cases if the formatting was done with a different version of `clang-format`. This has happened to me multiple times and for some reason always in a test. The bot should not complain if the formatting is fine in one of the previous n versions.

3. Tests are written in a different style optimised for conciseness. For example `struct S { S() = default; ~S() = delete; }; // not at all unusual in a test`. This could be accounted for in `.clang-format` but...

4. ...`clang-format-diff` does not respect (as far as I know?) the `.clang-format` in a sub-folder.

5. The lint bot is very noisy and makes reviews much harder because the formatting complaints are inline. Moving them out-of-line would be significant improvement.

I must admit I am very much tempted to put a generic `// clang-format off` in the tests I write.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83901