This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Properly forward clang-tidy output when running tests
ClosedPublic

Authored by nicovank on Jun 14 2022, 4:29 PM.

Details

Summary

When running tests, the check_clang_tidy script encodes the output string, making it hard to read when debugging checks. This removes the .encode() call.

Test Plan:
Making a new default check for testing (as of right now, it includes a failing test):

[~/llvm-project/clang-tools-extra] python3 clang-tidy/add_new_check.py bugprone example
<...>

Pre-changes:

[~/llvm-project/build] ninja check-clang-tools
<...>
------------------------ clang-tidy output -----------------------
b"1 warning generated.\n/data/users/nvankempen/llvm-project/build/Debug/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-example.cpp.tmp.cpp:4:6: warning: function 'f' is insufficiently awesome [bugprone-example]\nvoid f();\n     ^\n/data/users/nvankempen/llvm-project/build/Debug/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-example.cpp.tmp.cpp:4:6: note: insert 'awesome'\nvoid f();\n     ^\n     awesome_\n"

------------------------------------------------------------------
<...>

Post-changes:

[~/llvm-project/build] ninja check-clang-tools
<...>
------------------------ clang-tidy output -----------------------
1 warning generated.
/data/users/nvankempen/llvm-project/build/Debug/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-example.cpp.tmp.cpp:4:6: warning: function 'f' is insufficiently awesome [bugprone-example]
void f();
     ^
/data/users/nvankempen/llvm-project/build/Debug/tools/clang/tools/extra/test/clang-tidy/checkers/Output/bugprone-example.cpp.tmp.cpp:4:6: note: insert 'awesome'
void f();
     ^
     awesome_

------------------------------------------------------------------
<...>

Diff Detail

Event Timeline

nicovank created this revision.Jun 14 2022, 4:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 4:29 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript
nicovank retitled this revision from [clang-tidy] Remove encode when outputting tool ouput running tests to [clang-tidy] Properly forward clang-tidy output when running tests.Jun 14 2022, 4:32 PM
nicovank published this revision for review.Jun 14 2022, 5:01 PM
nicovank added reviewers: rnk, alexfh.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 5:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nicovank updated this revision to Diff 437163.Jun 15 2022, 7:32 AM

Looks like this causes the misc-misleading-identifier test to fail on the BuildKite Windows setup because it uses cp1252 and not utf-8. I think this is why the encode was added in the first place. This workaround should hopefully fix this, it will display unicode characters as '?' when stdout does not support utf-8.

Thank you for this, it has been annoying me for a while.
I'll give a tentative LG, but I'm no expert in this area so see what everyone else says first.

Ping, adding one more person who has history changing this script.

FWIW, I'm also a tentative LG on these changes. I'm not certain if there's a reason why we did things this way in the first place, but I think the new output is more readable.

Did some digging, looks like this was added in rG7f92a1a84b96, most likely to also fix that CI build error with misc-misleading-identifier.

Thanks for this, like Nathan James this has been pestering me for a while :)

This revision is now accepted and ready to land.Jul 1 2022, 10:49 AM
njames93 accepted this revision.Jul 1 2022, 1:10 PM

I'd say land this, but keep a close eye on the build bots as it may need reverting.

I do not have commit access for the LLVM repository, could one of you commit this for me (Nicolas van Kempen <nvankempen@fb.com>)?

I can keep an eye on this today, otherwise I will discuss with someone internally and commit Tuesday.