This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] allow clang-format to be passed a file of filenames so we can add a regression suite of "clean clang-formatted files" from LLVM
ClosedPublic

Authored by MyDeveloperDay on Oct 2 2021, 11:13 AM.

Details

Summary

When I make a change to clang-format I really want to run it over a large regression suite to ensure it doesn't cause any unexpected changes.

Building on the python script that creates the current status of all of LLVM, (see https://clang.llvm.org/docs/ClangFormattedStatus.html)
I now generate a list of "formatted files from LLVM" all 7900+ of them

This change now generates that list, and the change to clang-format allows us to run clang-format quickly over these files via the list of files.

/clang-format.exe -verbose -n --files=./clang/docs/tools/clang-formatted-files.txt

Clang-formating 7926 files
Formatting [1/7925] clang/bindings/python/tests/cindex/INPUTS/header1.h
Formatting [2/7925] clang/bindings/python/tests/cindex/INPUTS/header2.h
Formatting [3/7925] clang/bindings/python/tests/cindex/INPUTS/header3.h
....
Formatting [7923/7925] pstl/include/pstl/internal/parallel_backend_utils.h
Formatting [7924/7925] utils/bazel/llvm-project-overlay/clang/include/clang/Config/config.h
Formatting [7925/7925] utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/config.h

This is needed because putting all those files on the command line is too long, and invoking 7900+ clang-formats is much slower (too slow to be honest)

Using this method it takes on 7.5 minutes (on my machine) to run clang-format -n over all of the files (7925), this should result in us testing any change quickly and easily.

We should be able to use rerunning this list to ensure that we don't regress clang-format over a large code base, but also use it to ensure none of the previous files which were 100% clang-formatted remain so. (which the LLVM premerge checks should be enforcing)

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Oct 2 2021, 11:13 AM
MyDeveloperDay requested review of this revision.Oct 2 2021, 11:13 AM

Basically okay, I would have made 3 commits out of it:

  1. Add the function to clang-format
  2. The code clean up of the python script
  3. The additional file generation
This revision is now accepted and ready to land.Oct 2 2021, 12:56 PM

Basically okay, I would have made 3 commits out of it:

  1. Add the function to clang-format
  2. The code clean up of the python script
  3. The additional file generation

Yeah sorry about that, on their own I felt they were a little odd as to why I was making them, the formatting changes is because not I'm trying to run autopep8 and pylint

ychen added a subscriber: ychen.Sep 28 2022, 4:33 PM

It is possible to just use clang-format @response.txt? That should also invoke the executable once.

It is possible to just use clang-format @response.txt? That should also invoke the executable once.

It's true that whatever file you're handing to --files could just as easily be specified with @file.txt. --files could be removed and whatever script is driving it could be fixed accordingly. The argument to --files is really just a limited form of response file (you can't put options into it).
@MyDeveloperDay WDYT?

MyDeveloperDay added a comment.EditedSep 29 2022, 2:52 PM

The use of a response file was actually new to me, I suspect it isn't widely know about, but yes in theory it could be used. As this feature has been landed for sometime, I'm not inclined to remove it, there could be other people using it.

One does not need to remove this, when adding response file support.

Response files should work for all our tools, I think?

Keeping the option is fine with me, but the description needs some improvement. @ychen