Page MenuHomePhabricator

Removed superfluous and slightly annoying newlines in run-clang-tidy's output.
ClosedPublic

Authored by svenpanne on May 13 2019, 3:31 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

svenpanne created this revision.May 13 2019, 3:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2019, 3:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I agree with the direction, please ensure that e.g. clang-analyzer-* output looks proper as well and -quiet does, too.

[...] please ensure that e.g. clang-analyzer-* output looks proper as well and -quiet does, too.

OK, are there any (unit) tests which I should run? If yes, how exactly? I have a successfully built a fork of https://github.com/llvm/llvm-project via ninja locally, but the documentation for the various subprojects is still a bit hard to find to me. Is there a ninja target for clang-tidy-related tests? Any hints to get started would be highly appreciated.

This and the other output-related patches are my first submissions here, and they are intentionally simple to get the workflow right. My real intention is trying to improve/fix the readability-identifier-naming check/fixer, which still has a few issues.

[...] please ensure that e.g. clang-analyzer-* output looks proper as well and -quiet does, too.

OK, are there any (unit) tests which I should run? If yes, how exactly? I have a successfully built a fork of https://github.com/llvm/llvm-project via ninja locally, but the documentation for the various subprojects is still a bit hard to find to me. Is there a ninja target for clang-tidy-related tests? Any hints to get started would be highly appreciated.

This and the other output-related patches are my first submissions here, and they are intentionally simple to get the workflow right. My real intention is trying to improve/fix the readability-identifier-naming check/fixer, which still has a few issues.

There are testing targets for clang-tidy and clang-tooling, i believe ninja check-clang-tools is includes clang-tidy (maybe ninja check-clang-tooling instead).
If this script is actually covered by testing is not know to me, i believe not.
For testing just run it over a project (e.g. parts of LLVM) with this tool instead of e.g. your distribution version of it.

For general development: clang-tools-extra/test/clang-tidy/* has the test-cases for clang-tidy checks, which are real world code examples that cover all representative cases (in theory).

  1. Write/adjust test
  2. Do changes in code
  3. Test with ninja check-clang-tools / ninja check-clang-tooling

That usually works. If you encounter any issues feel free to ask here/IRC/mail, we are happy to help out :)

I think ninja clang-test-tools is the right target here, and it is happy with the change, which is not very surprising: AFAICT, there is just a single, very simple check for run-clang-tidy.py. Anyway, my manual testing with and without -quiet shows no problems, either. Furthermore, we have a patched version with the above change in our CI for quite some time, and it works fine. And if I read the git history correctly, the output should be identical to the one before a fix for interleaved output was landed (that fix added the newlines somehow).

JonasToth accepted this revision.May 16 2019, 4:48 AM

LGTM then, i can commit for you again?

This revision is now accepted and ready to land.May 16 2019, 4:48 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 5:39 AM

Thans for the patch!