This is an archive of the discontinued LLVM Phabricator instance.

This commit adds a chapter about clang-tidy integrations
ClosedPublic

Authored by MarinaKalashina on Nov 27 2018, 4:14 AM.

Details

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)

Diff Detail

Event Timeline

JonasToth added a project: Restricted Project.
JonasToth added subscribers: sammccall, 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

I think a blank line here would not hurt.

274

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

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

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 added inline comments.
docs/clang-tidy/index.rst
304

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

327

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

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 added a comment.Dec 4 2018, 3:37 AM

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 marked 4 inline comments as done.Dec 4 2018, 3:56 AM

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 added a comment.Dec 4 2018, 4:13 AM

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).

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

@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 added a comment.Dec 6 2018, 6:08 AM

@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 added a comment.Dec 6 2018, 6:11 AM

@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.

alexfh edited reviewers, added: ilya-biryukov; removed: asl.Dec 6 2018, 6:12 AM

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 set the repository for this revision to rCTE Clang Tools Extra.

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

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)

Full-context patch

@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.

Please run script from D55523 over your changes.

alexfh added inline comments.Dec 18 2018, 8:23 AM
docs/clang-tidy/index.rst
261

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)

@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
261

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 added a comment.Jan 7 2019, 8:08 AM

@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).

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

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

docs/clang-tidy/integrations.rst
118 ↗(On Diff #181018)

Please add newline.

integrations.rst - end of file new line

restoring full content

alexfh accepted this revision.Jan 11 2019, 2:50 PM

Awesome! Thanks a lot ! LG

This revision is now accepted and ready to land.Jan 11 2019, 2:50 PM

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 marked an inline comment as done.

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

@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

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.

@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?

This revision was automatically updated to reflect the committed changes.

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.

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.

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