[PATCH] D74877: [clang] fix incorrect Wdocumentation fix-its

[Alexander Lanin via Phabricator via llvm-commits]

AlexanderLanin marked an inline comment as not done.
AlexanderLanin added a comment.

> I'm not convinced the new approach is better in all cases, so I'm somewhat concerned about regressions from the current behaviour. Did you test this patch on a codebase to see the difference between the previous and new behaviour?

There are no warnings in llvm or ccache, all I found is a low quality codebase at work which does produce 1007 unique Wdocumentation warnings (see wc command).

It *seems* I can run this via:

  mkdir -p build-clang-D74877 && cd build-clang-D74877
  CC=/home/alex/llvm/build-D74877/bin/clang CXX=/home/alex/llvm/build-D74877/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync" -GNinja
  CC=/home/alex/llvm/build-D74877/bin/clang CXX=/home/alex/llvm/build-D74877/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync -fsyntax-only" -GNinja
  ninja -j1 -k10000 > warnings.txt
  grep "\[-Wdocumentation\]" warnings.txt | grep -v "empty paragraph passed to" | grep -v "^\[" | sort | uniq | wc -l
  cd ..
  
  mkdir -p build-clang-master && cd build-clang-master
  CC=/home/alex/llvm/build-master/bin/clang CXX=/home/alex/llvm/build-master/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync" -GNinja
  CC=/home/alex/llvm/build-master/bin/clang CXX=/home/alex/llvm/build-master/bin/clang++ cmake .. -DCMAKE_CXX_FLAGS="-Wdocumentation -Wno-documentation-deprecated-sync -fsyntax-only" -GNinja
  ninja -j1 -k10000 > warnings.txt
  grep "\[-Wdocumentation\]" warnings.txt | grep -v "empty paragraph passed to" | grep -v "^\[" | sort | uniq | wc -l
  cd ..
  
  # .. in order to leave git
  grep -v "^\[" build-clang-master/warnings.txt > ../clang-master-warnings.txt
  grep -v "^\[" build-clang-D74877/warnings.txt > ../clang-D74877-warnings.txt
  
  git diff ../clang-master-warnings.txt ../clang-D74877-warnings.txt

I do not know how to apply suggested fixes for an easier diff.
I could run clang-tidy with some inappropriate check, so that it will fix all the Wdocumentation warnings only. But that takes forever.

Anyway based on the approach above I found two additional regressions. Testing with real code seems to be kind of important ;-)

- Reported column numbers have been changed. Apparently this cannot be checked by expected-warning and expected-note, so I added another FileCheck pass with a few select test cases.
- All warnings are reported first and then all the notes. Apparently this is not checked by expected-warning and expected-note. This will probably also require to be checked by FileCheck directly.

I'll further analyze the diff ASAP (have to fix the second problem first so that the diff gets smaller).
But that experience was kind of enough to get the following idea:
I would like to create two different patches:

- One with all the new tests (modified to pass based on current code). It should be trivial to review.
- One with the new code and the required small modifications to tests.

Because at the moment lots of new test cases are added and it's not clear what is even different now with this patch.
That seems to be the best way to ensure all changes are clearly visible within test cases.

What do you think of that @Mordante?


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

https://reviews.llvm.org/D74877