This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy docs] Add missing option docs.
ClosedPublic

Authored by hokein on Aug 26 2016, 4:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hokein updated this revision to Diff 69342.Aug 26 2016, 4:41 AM
hokein retitled this revision from to [clang-tidy docs] Add missing options..
hokein updated this object.
hokein added a subscriber: cfe-commits.
hokein updated this revision to Diff 69343.Aug 26 2016, 4:46 AM

Fix style.

hokein retitled this revision from [clang-tidy docs] Add missing options. to [clang-tidy docs] Add missing option docs..Aug 26 2016, 4:48 AM
hokein added reviewers: alexfh, Eugene.Zelenko.
hokein removed a subscriber: nemanjai.

General comments:

  • I think default is XYZ is better then XYZ by default.
  • There are discrepancies: option representing and option represents.

I think will be good idea to involve native English speaker in review.

docs/clang-tidy/checks/google-readability-namespace-comments.rst
11 ↗(On Diff #69343)

This documentation is redirect, so options description should be moved to llvm-namespace-comment.rst.

14 ↗(On Diff #69343)

Unnecessary semicolon at the end. Same below.

docs/clang-tidy/checks/google-runtime-int.rst
27 ↗(On Diff #69343)

Will empty string displayed? May be it should be expressed in words?

docs/clang-tidy/checks/misc-move-constructor-init.rst
22 ↗(On Diff #69343)

Unnecessary empty line.

26 ↗(On Diff #69343)

s/checker/check/. I think will be good idea to provide link to cert-oop11-cpp.rst.

docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
49 ↗(On Diff #69343)

s/checker/check/

docs/clang-tidy/checks/misc-suspicious-string-compare.rst
55 ↗(On Diff #69343)

Will empty string displayed? May be it should be expressed in words?

docs/clang-tidy/checks/modernize-replace-auto-ptr.rst
73 ↗(On Diff #69343)

Unnecessary empty string.

79 ↗(On Diff #69343)

llvm should be on this line. See other similar descriptions.

docs/clang-tidy/checks/performance-inefficient-string-concatenation.rst
57 ↗(On Diff #69343)

Please highlight 0 with `.

docs/clang-tidy/checks/readability-implicit-bool-cast.rst
106 ↗(On Diff #69343)

Looks like 0 by should fit 80 characters. Same below.

hokein updated this revision to Diff 69538.Aug 29 2016, 1:24 AM
hokein marked 8 inline comments as done.
hokein edited edge metadata.

Address review comments:

  • Use "Default is XYZ" instead of "XYZ by default".
  • Replace empty string `` with empty word, passing docs compilation.
hokein updated this revision to Diff 69539.Aug 29 2016, 1:27 AM

Remove extra blank line.

docs/clang-tidy/checks/google-readability-namespace-comments.rst
11 ↗(On Diff #69343)

Oh, didn't notice it. Done.

docs/clang-tidy/checks/modernize-replace-auto-ptr.rst
79 ↗(On Diff #69343)

If we put it into one line, the line's length will exceed 80 characters.

docs/clang-tidy/checks/readability-implicit-bool-cast.rst
106 ↗(On Diff #69343)

It exceeds 80 characters by an extra character...

docs/clang-tidy/checks/google-runtime-int.rst
27 ↗(On Diff #69539)

I think empty string will be better. Same for other occurrences.

hokein updated this revision to Diff 69679.Aug 30 2016, 7:40 AM
hokein marked an inline comment as done.
  • Use empty string words.
  • rebase to master.

I don't have further comments, but as non-English speaker and mediocre writer, I would like see other people comments. May be @aaron.ballman could help if Alexander is busy?

aaron.ballman requested changes to this revision.Aug 30 2016, 1:15 PM
aaron.ballman added a reviewer: aaron.ballman.

Thank you for working on this!

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

s/represents/specifying

docs/clang-tidy/checks/google-build-namespaces.rst
20 ↗(On Diff #69679)

s/not contain/not include the

21 ↗(On Diff #69679)

How about:

For header files without an extension, use an empty string (if there are no other desired extensions) or leave an empty element in the list. e.g., "h,hh,hpp,hxx," (note the trailing comma).

(assuming my example actually works.)

docs/clang-tidy/checks/google-runtime-int.rst
19 ↗(On Diff #69679)

A string specifying the unsigned type prefix.

23 ↗(On Diff #69679)

A string specifying the signed type prefix.

27 ↗(On Diff #69679)

A string specifying the type suffix.

docs/clang-tidy/checks/llvm-namespace-comment.rst
20 ↗(On Diff #69679)

How about:

Requires the closing brace of the namespace definition to be followed by a closing comment if the body of the namespace has more than `ShortNamespaceLines` lines of code. The value is an unsigned integer that defaults to `1U`.
25 ↗(On Diff #69679)

How about:

An unsigned integer specifying the number of spaces before the comment closing a namespace definition.
docs/clang-tidy/checks/misc-definitions-in-headers.rst
83 ↗(On Diff #69679)

Same suggestion here as above.

89 ↗(On Diff #69679)

s/use use/use the

docs/clang-tidy/checks/misc-move-constructor-init.rst
20 ↗(On Diff #69679)

s/represents/specifying

25–26 ↗(On Diff #69679)

cert-oop11-cpp redirects to this file, so the link is a bit circular. Perhaps a better link is:

How about:

When non-zero, the check conforms to the behavior expected by the CERT secure coding recommendation `OOP11-CPP <https://www.securecoding.cert.org/confluence/display/cplusplus/OOP11-CPP.+Do+not+copy-initialize+members+or+base+classes+from+a+move+constructor>`_. Default is `0` for misc-move-constructor-init and `1` for cert-oop11-cpp
docs/clang-tidy/checks/misc-sizeof-expression.rst
142 ↗(On Diff #69679)

s/on/on an

147 ↗(On Diff #69679)

s/on/on an

152 ↗(On Diff #69679)

s/on/on an

153 ↗(On Diff #69679)

It would be good to describe what a suspicious constant actually is.

docs/clang-tidy/checks/misc-string-constructor.rst
39 ↗(On Diff #69679)

How about:

.. warn on a string with a length greater than `LargeLengthThreshold`.
44 ↗(On Diff #69679)

s/represents/specifying

Also, the value is wrong, the default is 0x800000 not 0x8000000.

docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
48 ↗(On Diff #69679)

s/represents minimal/specifying the minimum
s/literals array/literal

53 ↗(On Diff #69679)

represents->specifying
maximal->maximum

Should actually state that the string represents a floating-point value between 0 and 1 (inclusive? exclusive?). Also, is it really a string? aka, does it need quotes, or is it okay to not use quotes (as the default suggests)?

58 ↗(On Diff #69679)

s/represents maximal/specifying the maximum

docs/clang-tidy/checks/misc-suspicious-string-compare.rst
53 ↗(On Diff #69679)

How about:

A string specifying the comma-separated names of string comparison functions.

We should also list the default string comparison functions.

docs/clang-tidy/checks/modernize-pass-by-value.rst
160 ↗(On Diff #69679)

s/represents/specifying

docs/clang-tidy/checks/modernize-replace-auto-ptr.rst
78 ↗(On Diff #69679)

s/represents/specifying

docs/clang-tidy/checks/performance-for-range-copy.rst
26 ↗(On Diff #69679)

What is an "auto copy"? I think this option just warns if auto is used as the type specifier in a range-based for loop, so perhaps this should just say "When non-zero, warns on any use of auto as the type of the range-based for loop variable."

docs/clang-tidy/checks/performance-unnecessary-value-param.rst
62 ↗(On Diff #69679)

s/represents/specifying

This revision now requires changes to proceed.Aug 30 2016, 1:15 PM
hokein updated this revision to Diff 69812.Aug 31 2016, 1:46 AM
hokein edited edge metadata.
hokein marked 26 inline comments as done.

Address aaron's comments.

wow, thanks for many detailed comments.

docs/clang-tidy/checks/google-build-namespaces.rst
21 ↗(On Diff #69679)

The trailing comma in h,hh,hpp,hxx, is not required here.

docs/clang-tidy/checks/misc-suspicious-missing-comma.rst
53 ↗(On Diff #69679)

Yeah, this is a string in the check's implementation.

aaron.ballman added inline comments.Aug 31 2016, 5:42 AM
docs/clang-tidy/checks/google-build-namespaces.rst
21 ↗(On Diff #69812)

I may be confused then -- does the string "h,hh,hpp,hxx" (no trailing comma) also wind up including header files without extensions? If so, we should document that. If not, would adding a trailing comma cause it to include header files without an extension? If so, that's what my example was trying to point out, but perhaps my phrasing needs clarifying.

docs/clang-tidy/checks/misc-sizeof-expression.rst
154 ↗(On Diff #69812)

s/bigger than/greater than

docs/clang-tidy/checks/performance-for-range-copy.rst
26 ↗(On Diff #69812)

Should quote auto with backtics, I suppose.

hokein updated this revision to Diff 69838.Aug 31 2016, 5:57 AM
hokein marked 3 inline comments as done.
hokein edited edge metadata.

Fix more comments.

docs/clang-tidy/checks/google-build-namespaces.rst
21 ↗(On Diff #69812)

Aha, sorry, I misunderstood your words here. Yeah, we should add trailing , for the non-extension headers.

aaron.ballman added inline comments.Aug 31 2016, 6:01 AM
docs/clang-tidy/checks/google-build-namespaces.rst
21 ↗(On Diff #69838)

Okay, that make sense to me then. This change should also be applied to other places that have the extension list.

hokein updated this revision to Diff 69839.Aug 31 2016, 6:10 AM

Update HeaderFileExtensions doc for all documents.

hokein marked an inline comment as done.Aug 31 2016, 6:11 AM
hokein added inline comments.
docs/clang-tidy/checks/google-build-namespaces.rst
21 ↗(On Diff #69839)

All related documents have been updated now.

aaron.ballman accepted this revision.Aug 31 2016, 6:18 AM
aaron.ballman edited edge metadata.

LGTM, thank you for the updated documentation!

This revision is now accepted and ready to land.Aug 31 2016, 6:18 AM
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.