This is an archive of the discontinued LLVM Phabricator instance.

[Clang-tidy] Fix style in some checks documentation
ClosedPublic

Authored by Eugene.Zelenko on Aug 19 2016, 3:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Eugene.Zelenko retitled this revision from to [Clang-tidy] Fix style in some checks documentation.
Eugene.Zelenko updated this object.
Eugene.Zelenko added a reviewer: alexfh.
Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a subscriber: cfe-commits.
alexfh requested changes to this revision.Aug 20 2016, 10:01 AM
alexfh edited edge metadata.
alexfh added inline comments.
docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
11 ↗(On Diff #68739)

I don't see the rest of the file (upload full diffs, btw), but we need to add .. option:: GslHeader, if it's not there yet. And I think, the general rule should be to add option description in every check that uses the option, since different checks can use the same global option in a subtly different ways.

I hope, Sphinx supports this use case.

This revision now requires changes to proceed.Aug 20 2016, 10:01 AM

Please rebase your patch, I've made some changes to some of these files.

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst
39 ↗(On Diff #68739)

The .. option:: CheckImplicitCasts block should be added in this document to prevent Sphinx warnings. In order to verify this, you should install Sphinx 1.4.5 (on linux this can be done using pip install -u Sphinx, not sure about Windows).

57 ↗(On Diff #68739)

No space after the closing parenthesis is needed. Same above.

Eugene.Zelenko edited edge metadata.

Address review comments.

alexfh accepted this revision.Aug 22 2016, 3:08 PM
alexfh edited edge metadata.

LG with a few nits.

docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
9 ↗(On Diff #68908)

Can we make this a link?

19 ↗(On Diff #68908)

nit: "the this" - one of these should go

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst
19 ↗(On Diff #68908)

x * 1000 should also be enclosed in double quotes, imo.

65 ↗(On Diff #68908)

I'd say "If non-zero, enables detection of implicit casts. Default is non-zero."

This revision is now accepted and ready to land.Aug 22 2016, 3:08 PM
Eugene.Zelenko edited edge metadata.

Address review comments.

Eugene.Zelenko added inline comments.Aug 22 2016, 3:36 PM
docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
9 ↗(On Diff #68921)

Unfortunately not. Historically Clang warnings options documentation is very poor in comparison with GCC's.

Probably some descriptions could be extracted from release notes, but I'm not sure that clang-diagnostic-array-bounds is described there.

alexfh added inline comments.Aug 22 2016, 4:35 PM
docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst
9 ↗(On Diff #68921)

Ah, I completely missed the fact it's a clang diagnostic. The wording is off here. Should be "see the -Warray-bounds clang diagnostic" instead. The fact that clang diagnostics map to clang-diagnostic-... is described in the main doc (http://clang.llvm.org/extra/clang-tidy/).

Address review comment.

Address review comment.

Thanks! Still LG.

This revision was automatically updated to reflect the committed changes.