This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy][doc] Improve clang-tidy documentation
ClosedPublic

Authored by carlosgalvezp on Jan 6 2023, 9:55 AM.

Details

Summary
  • Specify that the .clang-tidy file is in YAML format.
  • Document the options that may be used in the .clang-tidy file,
  • Add missing documentation for existing options (User).
  • Fix spurious newline after the dash that comes after every command-line option. This was inconsistent with single-line descriptions, which lacked a newline. The description is now aligned with the dash and the corresponding command-line option, more visually pleasing.

This enables documenting upcoming global clang-tidy
configuration options.

Diff Detail

Event Timeline

carlosgalvezp created this revision.Jan 6 2023, 9:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
carlosgalvezp requested review of this revision.Jan 6 2023, 9:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 9:55 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Remove extra newline

Remove wrong dash in key/value options.

Fix incorrect dash in key/value pairs

Remove colon after the config options, for consistency with
the command-line options.

Fix alignment

Eugene.Zelenko accepted this revision.Jan 6 2023, 10:11 AM
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/clang-tidy/index.rst
140–142

Is this merge artifact? Same below.

215

Ditto.

This revision is now accepted and ready to land.Jan 6 2023, 10:11 AM
Eugene.Zelenko requested changes to this revision.Jan 6 2023, 10:11 AM
This revision now requires changes to proceed.Jan 6 2023, 10:11 AM
carlosgalvezp marked 2 inline comments as done.Jan 6 2023, 10:31 AM
carlosgalvezp added inline comments.
clang-tools-extra/docs/clang-tidy/index.rst
140–142

I honestly have no idea, it seems some artifact of Phabricator that is unrelated to the code which is just fine. It's not a tab either, checked with my editor. I have seen it in patches from other people as well, leading to confusion...

Eugene.Zelenko accepted this revision.Jan 6 2023, 10:36 AM

But will be good idea to wait for other reviewers.

This revision is now accepted and ready to land.Jan 6 2023, 10:36 AM
carlosgalvezp marked an inline comment as done.

Fix formatting

Trim first character instead, to keep the code visually
pleasing.

@njames93 Do you agree with the overall idea of documenting config file options like this? I need this in place for the other patch that cleans the duplication for HeaderFileExtensions, ImplementationFileExtensions, IncludeStyle, etc.

njames93 added inline comments.Jan 10 2023, 9:03 AM
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
34

This seems a bit of a needless change, if you want to remove the newline, just in date the raw string literal and remove the empty line

55

Can this be changed to match the previous format defined below

84

This change can be reverted.

clang-tools-extra/docs/clang-tidy/index.rst
261

Again as before

292

Ditto

carlosgalvezp marked 3 inline comments as done.

Revert changes to CheckOptions

clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
34

I'm not sure I understand what you mean by "just in date the raw string", could you clarify?

In Diff 7 I fixed this without TrimFirstChar, but it leads to fairly inconsistent code, where the first R"( needs to be part of the first line of text:

R"(some text
some more text
)"

versus:

R"(
some text
some more text
)"

Let me know which option you prefer.

84

Thanks, I didn't know it was another valid way to write it! Will fix

Add ExtraArgs and ExtraArgsBefore

Add HeaderFileExtensions and ImplementationFileExtensions

Fix formatting

carlosgalvezp edited the summary of this revision. (Show Details)

Remove newline

Friendly ping @njames93 , see my previous comment about TrimFirstChar.

PiotrZSL added inline comments.
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
34

Looks like usecase of this is more like rtrim, then probably rtrim should be used instead of substr.
And probably better would be to replace entire cl::desc. instead of adding new additional wrap.

more like:
static auto desc(llvm::StringRef description) { return cl::desc(description.rtrim()); }

Wrap trimming functionality in desc function.

carlosgalvezp marked an inline comment as done.Jan 29 2023, 11:16 AM
carlosgalvezp added inline comments.
clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
34

Really good idea, much better! Done.

carlosgalvezp marked an inline comment as done.Feb 1 2023, 11:22 PM

All comments have been addressed, I intend to land the patch by February 9th unless more feedback is received.

This revision was automatically updated to reflect the committed changes.
PiotrZSL added inline comments.May 4 2023, 3:52 AM
clang-tools-extra/docs/clang-tidy/index.rst
272

This is a bug, SystemHeaders cannot be specified on this level, as they not added to MappingTraits<ClangTidyOptions>. Therefor listing them here is a misleading.