Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

check_clang_tidy.py uses git diff but doesnt document it #30867

Open Quuxplusone opened 7 years ago

Quuxplusone commented 7 years ago
Bugzilla Link PR31894
Status NEW
Importance P enhancement
Reported by Alexander Lanin (llvm@alex.lanin.de)
Reported on 2017-02-07 15:03:25 -0800
Last modified on 2017-02-09 05:04:46 -0800
Version unspecified
Hardware PC Windows NT
CC alexfh@google.com, djasper@google.com, klimek@google.com
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
It's rather non obvious what diff command is supposed to be invoked in
check_clang_tidy.py

I tried some and finally git diff seems right, however this is not documented
anywhere as far as I can tell. I have never written any python code, so all I
can suggest on my part is adding a comment like this one in check_clang_tidy.py
around line 100:

  try:
    # this refers to gits diff command, make sure it's in your PATH
    diff_output = subprocess.check_output(
        ['diff', '-u', original_file_name, temp_file_name],
        stderr=subprocess.STDOUT)
Quuxplusone commented 7 years ago

I suppose, you stumbled upon this on Windows or OSX?

The script needs GNU diff (a part of diffutils package, should be available via gnuwin32 as well). Specifically, its unified diff format (https://www.gnu.org/software/diffutils/manual/html_node/Unified-Format.html). But that's just a convenience feature to help diagnose test failures. Any other text-based diff command could be used instead.

However, a few lit tests in clang-tools-extra use GNU diff as a part of their normal workflow (I found clang-tidy/clang-tidy-diff.cpp, clang-tidy/misc-unused-parameters.cpp, clang-tidy/clang-tidy-diff.cpp, clang-apply-replacements/crlf.cpp, test/include-fixer/merge.test), so it might make sense to update the documentation instead of adding comments to each of these tests.