This commit adds a chapter about clang-tidy integrations

[MarinaKalashina]

Summary

Proposing changes to clang-tools-extra/docs/clang-tidy/index.rst

'Using clang-tidy' was split into 2 chapters:

• 'Standalone tool' (the previous content of 'Using clang-tidy')
• 'Clang-tidy integrated' (a new chapter)

[JonasToth]

I like the overview, maybe a link to clangd here might help, as there is currently a lot of effort of integrating clang-tidy into it. (@sammccall WDYT?)

docs/clang-tidy/index.rst

23 ↗

(On Diff #175452)

I think a blank line here would not hurt.

274 ↗

(On Diff #175452)

Could you please align the elements in the columns, some are off by one.

[alexfh]

Should this chapter be a separate page linked from index.md?

[sammccall]

In D54945#1309467, @JonasToth wrote:

I like the overview, maybe a link to clangd here might help, as there is currently a lot of effort of integrating clang-tidy into it. (@sammccall WDYT?)

This would be great! The integration is not yet complete, so I'm not sure if it's best to add now or later.

For the table:

• on-the-fly inspection: yes (I think - this means showing fixes overlaid on the code as you edit, right?)
• check list configuration (GUI): no
• options to checks (GUI): no
• configuration via .clang-tidy file: this will be the supported mechanism. Not done yet.
• custom clang-tidy binary: no

The behavior today is that three hard-coded checks are always enabled, as a proof-of-concept. Configuration via .clang-tidy is the main missing piece.
Clang-tidy diagnostics are shown along with others, as-you-type. Fixes can be applied.
Static analyzer checks will probably never be supported.

[Eugene.Zelenko]

docs/clang-tidy/index.rst

304 ↗

(On Diff #175452)

Please limit line width to 80 characters. Same in other places.

327 ↗

(On Diff #175452)

Will be good idea to mention native plugin, clang-tidy-vs.

[MarinaKalashina]

Fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)

Additions:

• clang-tidy-vs plugin
• Clangd in the intro, the table, and CLion's paragraph

[alexfh]

In D54945#1316251, @MarinaKalashina wrote:

Fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)

Additions:

• clang-tidy-vs plugin
• Clangd in the intro, the table, and CLion's paragraph

Did you forget to add a new file to the patch?

Please also include full context into the diff. See https://llvm.org/docs/Phabricator.html

[MarinaKalashina]

In D54945#1318278, @alexfh wrote:

In D54945#1316251, @MarinaKalashina wrote:

Fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)

Additions:

• clang-tidy-vs plugin
• Clangd in the intro, the table, and CLion's paragraph

Did you forget to add a new file to the patch?

Please also include full context into the diff. See https://llvm.org/docs/Phabricator.html

Sorry but the new diff was included to the patch.. I can see the updates as Diff 176328. Could you please check and let me know if it does not work? Thank you.

[alexfh]

In D54945#1318283, @MarinaKalashina wrote:

In D54945#1318278, @alexfh wrote:

In D54945#1316251, @MarinaKalashina wrote:

Fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)

Additions:

• clang-tidy-vs plugin
• Clangd in the intro, the table, and CLion's paragraph

Did you forget to add a new file to the patch?

Please also include full context into the diff. See https://llvm.org/docs/Phabricator.html

Sorry but the new diff was included to the patch.. I can see the updates as Diff 176328. Could you please check and let me know if it does not work? Thank you.

Please see the screenshot. The diff might be reversed (new vs. old) or just wrong. And there's no context (not overly important in this particular patch, but still makes it more convenient to review).

[MarinaKalashina]

Full-context patch for the changes introduced in the previous commit.

[MarinaKalashina]

@alexfh Thanks a lot for your patience and help. I've made another revision, now with the diff made by 'git show HEAD -U999999' to have the full context availlable.

[alexfh]

In D54945#1318343, @MarinaKalashina wrote:

@alexfh Thanks a lot for your patience and help. I've made another revision, now with the diff made by 'git show HEAD -U999999' to have the full context availlable.

I don't know how this happened, but the diff is still almost empty. This is what I get when I click on the "Download raw diff":

Index: docs/clang-tidy/index.rst
===================================================================
--- docs/clang-tidy/index.rst
+++ docs/clang-tidy/index.rst
@@ -403,7 +403,7 @@
 
     // Silent only the specified diagnostics for the next line
     // NOLINTNEXTLINE(google-explicit-constructor, google-runtime-int)
-    Foo(bool param);
+    Foo(bool param); 
   };
 
 The formal syntax of ``NOLINT``/``NOLINTNEXTLINE`` is the following:

It's definitely not a common problem, since there are other patches in review, which are displayed just fine.

[alexfh]

In D54945#1321301, @alexfh wrote:

In D54945#1318343, @MarinaKalashina wrote:

@alexfh Thanks a lot for your patience and help. I've made another revision, now with the diff made by 'git show HEAD -U999999' to have the full context availlable.

I don't know how this happened, but the diff is still almost empty.

A correction: it's now empty (and it was broken in a different way last time).

Could you verify that you're correctly following one of the processed described in the documentation (https://llvm.org/docs/Phabricator.html)? It looks like you're uploading only one commit from the whole branch.

[ilya-biryukov]

Maybe try to use Arcanist for uploading diffs?
The web interface can be confusing at times, Arcanist is more reliable (LLVM docs have pointers to the Arcanist documentation to get started)

[MarinaKalashina]

Another attempt to update the patch with the following fixes:

• empty line before 'Standalone tool'
• table columns with '+/-' aligned
• line width limited to 80 (except for the table)
• clang-tidy-vs plugin added
• Clangd added to the intro, the table, and the CLion's paragraph

[ilya-biryukov]

Thanks for the update! We still need to solve the technicality here: could you please upload the diff with full context? See LLVM docs for an example on how this can be done with various version controls (the -U99999 args are important, they are the ones that produce diffs with enough lines around it)

[MarinaKalashina]

Full-context patch

[MarinaKalashina]

@ilya-biryukov Thank you for a quick response and sorry for so many iterations. I uploaded a full-context version; hopefully this time it's alright - on my side, it finally looks like a proper full-context diff.

[Eugene.Zelenko]

Please run script from D55523 over your changes.

[alexfh]

docs/clang-tidy/index.rst

262 ↗

(On Diff #178647)

So how about moving this chapter to a separate page and adding a link to it at the top of this document? While certainly useful, the information in this section has a different focus from the rest of the documentation. Given that the document has grown significantly, I suggest to split out this section and the "Getting involved" section. (The latter can be done separately)

[MarinaKalashina]

@alexfh Just for me to be sure, should there be the following structure in http://clang.llvm.org/extra/index.html:
Clang-Tidy

The list of clang-tidy checks
Using clang-tidy
Suppressing Undesired Diagnostics
Clang-tidy Integrated (separate page)
Getting Involved (separate page)

And should the latter two pages be linked from 'Clang-Tidy' index.rs like 'The list of clang-tidy checks', which is placed in 'See also'?
Thank you.

docs/clang-tidy/index.rst

262 ↗

(On Diff #178647)

Just for me to be sure, should there be the following structure in http://clang.llvm.org/extra/index.html:
Clang-Tidy

The list of clang-tidy checks
Using clang-tidy
Suppressing Undesired Diagnostics
Clang-tidy Integrated (separate page)
Getting Involved (separate page)

And should the latter two pages be linked from 'Clang-Tidy' index.rs like 'The list of clang-tidy checks', which is placed in 'See also'?
Thank you.

[alexfh]

In D54945#1340528, @MarinaKalashina wrote:

@alexfh Just for me to be sure, should there be the following structure in http://clang.llvm.org/extra/index.html: ...

Roughly like that. More specifically, I'd suggest to modify the See also section of clang-tidy/index.rst as follows:

See also:

.. toctree::
   :maxdepth: 1

   The list of clang-tidy checks <checks/list>
   Clang-tidy IDE/Editor Integrations <integrations>
   Getting Involved <contribution>

The order of TOC items in http://clang.llvm.org/extra/index.html will be slightly weird (the three external docs will be placed before the headings of the main document), but that doesn't seem like a huge problem (and may be fixable).

[MarinaKalashina]

Extracting 'Getting Involved' and 'Clang-tidy integrations' to separate pages linked in See Also.

[MarinaKalashina]

@alexfh Thank you, please see the updated structure.
@Eugene.Zelenko Done, the only warnings I got were about the table rows width.

[Eugene.Zelenko]

docs/clang-tidy/integrations.rst

118

Please add newline.

[MarinaKalashina]

integrations.rst - end of file new line

[MarinaKalashina]

restoring full content

[alexfh]

Awesome! Thanks a lot ! LG

[Eugene.Zelenko]

I would suggest to rename contribution to Contributing (see LLVM documentation) and integrations to Integrations.

Will be also good idea to fix D55523 script warnings.

[MarinaKalashina]

files renamed: contribution.rst to Contributing.rst, integrations.rst to Integrations.rst

[MarinaKalashina]

@Eugene.Zelenko

I would suggest to rename contribution to Contributing (see LLVM documentation) and integrations to Integrations.

Sure, done.

Will be also good idea to fix D55523 script warnings.

The script warns about double spaces and line width in the table, for example:

warning: line 39 contains double spaces
warning: line 39 is in excess of 80 characters (196)

These spaces and width are intended since they 'draw' the table layout. Do you think we could ignore such warnings in this particular case?

[Eugene.Zelenko]

In D54945#1355920, @MarinaKalashina wrote:

@Eugene.Zelenko

I would suggest to rename contribution to Contributing (see LLVM documentation) and integrations to Integrations.

Sure, done.

Will be also good idea to fix D55523 script warnings.

The script warns about double spaces and line width in the table, for example:

warning: line 39 contains double spaces
warning: line 39 is in excess of 80 characters (196)

These spaces and width are intended since they 'draw' the table layout. Do you think we could ignore such warnings in this particular case?

Sure, it's fine to leave table as it is. Same thing is done in LLDB code where tables are excluded from Clang-format.

[MarinaKalashina]

@Eugene.Zelenko @alexfh Thank you! How should I proceed with the revision now? Do I get it right from https://llvm.org/docs/Phabricator.html#committing-a-change, that without commit access, I need to ask for committing the changes for me?

[Eugene.Zelenko]

clang-tools-sphinx-docs bot is failing because of:

Warning, treated as error:
/home/buildbot/llvm-build-dir/clang-tools-sphinx-docs/llvm/src/tools/clang/tools/extra/docs/clang-tidy/Contributing.rst:61: ERROR: Unknown target name: "how to setup tooling for llvm".

Frankly I don't know what is proper way to handle cross-module documentation links.

[Eugene.Zelenko]

I fixed links.

[Eugene.Zelenko]

By the word, I noticed that HTTP was used and replaced it with HTTPS in Contributing.rst. Will be good idea to do the same in other documentation.

[Eugene.Zelenko]

I requested merge of updated documentation into 8.0 branch in PR40369.

docs/clang-tidy/integrations.rst

 .. _CodeChecker: https://github.com/Ericsson/codechecker
 .. _plugin: https://github.com/Ericsson/CodeCheckerEclipsePlugin
 
 :program:`clang-tidy` is integrated in CPPDepend_ starting from version 2018.1
 and CPPCheck_ 1.82. CPPCheck integration lets you import Visual Studio
 solutions and run the :program:`clang-tidy` inspection on them. The
 CodeChecker_ application of version 5.3 or later, which also comes as a plugin_
 for Eclipse, supports :program:`clang-tidy` as a static analysis instrument and
-allows to use a custom :program:`clang-tidy` binary.
+allows to use a custom :program:`clang-tidy` binary.