This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] update --files help description
ClosedPublic

Authored by ychen on Oct 3 2022, 3:34 PM.

Diff Detail

Event Timeline

ychen created this revision.Oct 3 2022, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 3:34 PM
ychen requested review of this revision.Oct 3 2022, 3:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 3 2022, 3:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
MyDeveloperDay added inline comments.Oct 4 2022, 5:39 AM
clang/docs/ClangFormat.rst
29

if response files are so common place why not say it here

clang-format [options] [@file]

I personally don't believe we need to document how a shell works

72

if you google "response files" you don't get anything that leads to understand what they are. I understand what it means but I'm not convinced others who have not come across it before will. I don't personally think this change adds anything.

MyDeveloperDay added a project: Restricted Project.

The option handling in clang-format itself would need to be updated as well.

clang/docs/ClangFormat.rst
29

Pretty sure it's not the shell doing the expansion here? Certainly Clang has explicit code for deciding between gnu-style and windows-style handling.

Its Is a minor point, but when using --files, the shell will allow me to autocomplete..

 clang-format -verbose -n -files ./cla<TAB>
 clang-format -verbose -n -files ./clang/d<TAB>
 etc...

all the way to the file
 clang-format -verbose -n -files ./clang/docs/tools/clang-formatted-files.txt

with @ I have to be pixel perfect in my path.. so I'm more likely to go wrong

clang-format -verbose -n @./cla<TAB> does nothing.

that feels like a positive for the --files.

probinson added inline comments.Oct 4 2022, 8:32 AM
clang/docs/ClangFormat.rst
72–73

I was thinking something like this. In particular, call the argument a "filename" and then say what it should look like. The old description with a "string" argument reads like it might want to be
--files=file1,file2,file3
or something like that.

ychen updated this revision to Diff 465134.Oct 4 2022, 1:13 PM
  • address comments

The help text in ClangFormat.cpp for the --files option needs to be updated the same way.
Note using cl::value_desc("filename") is what you need to change the meta-variable in the help output.

ychen updated this revision to Diff 465151.Oct 4 2022, 2:02 PM
  • make corresponding change in the commandline

It's too long ago for me. Does the --files option take multiple file names on the command line, or a file containing the file names? If the latter the patch generally looks good.

Except for the @<file> part. (Which also isn't handles by clang-format, or is it?)

It's too long ago for me. Does the --files option take multiple file names on the command line, or a file containing the file names? If the latter the patch generally looks good.

It wants the filename of a text file containing a list of filenames, one per line.

ychen added a comment.Oct 6 2022, 10:11 AM

Except for the @<file> part. (Which also isn't handles by clang-format, or is it?)

It is not. It is handled by the LLVM commandline library. https://llvm.org/docs/CommandLine.html#response-files

This revision is now accepted and ready to land.Oct 7 2022, 1:40 AM
This revision was landed with ongoing or failed builds.Oct 11 2022, 10:25 AM
This revision was automatically updated to reflect the committed changes.
owenpan added inline comments.Sep 5 2023, 4:04 PM
clang/docs/ClangFormat.rst
29

We should not modify the file directly. See below.

72–74

It should be edited in ClangFormat.cpp as this file is generated by dump_format_help.py.

Herald added a subscriber: wangpc. · View Herald Transcript
NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

owenpan added inline comments.Sep 7 2023, 12:48 AM
clang/docs/ClangFormat.rst
72–74

Fixed in c47c480b1845.