Details
Diff Detail
Event Timeline
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 | Will empty string displayed? May be it should be expressed in words? | |
docs/clang-tidy/checks/misc-move-constructor-init.rst | ||
22 | Unnecessary empty line. | |
26 | 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 | s/checker/check/ | |
docs/clang-tidy/checks/misc-suspicious-string-compare.rst | ||
55 | Will empty string displayed? May be it should be expressed in words? | |
docs/clang-tidy/checks/modernize-replace-auto-ptr.rst | ||
73 | Unnecessary empty string. | |
79 | llvm should be on this line. See other similar descriptions. | |
docs/clang-tidy/checks/performance-inefficient-string-concatenation.rst | ||
57 | Please highlight 0 with `. | |
docs/clang-tidy/checks/readability-implicit-bool-cast.rst | ||
106 | Looks like 0 by should fit 80 characters. Same below. |
Address review comments:
- Use "Default is XYZ" instead of "XYZ by default".
- Replace empty string `` with empty word, passing docs compilation.
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 | 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 | It exceeds 80 characters by an extra character... |
docs/clang-tidy/checks/google-runtime-int.rst | ||
---|---|---|
27 | I think empty string will be better. Same for other occurrences. |
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?
Thank you for working on this!
docs/clang-tidy/checks/cppcoreguidelines-pro-bounds-constant-array-index.rst | ||
---|---|---|
24 | s/represents/specifying | |
docs/clang-tidy/checks/google-build-namespaces.rst | ||
20 | s/not contain/not include the | |
21 | 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 | A string specifying the unsigned type prefix. | |
23 | A string specifying the signed type prefix. | |
27 | A string specifying the type suffix. | |
docs/clang-tidy/checks/llvm-namespace-comment.rst | ||
20 | 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 | 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 | Same suggestion here as above. | |
89 | s/use use/use the | |
docs/clang-tidy/checks/misc-move-constructor-init.rst | ||
20 | s/represents/specifying | |
25–26 | 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 | ||
144 | s/on/on an | |
149 | s/on/on an | |
154 | s/on/on an | |
155 | It would be good to describe what a suspicious constant actually is. | |
docs/clang-tidy/checks/misc-string-constructor.rst | ||
39 | How about: .. warn on a string with a length greater than `LargeLengthThreshold`. | |
44 | s/represents/specifying Also, the value is wrong, the default is 0x800000 not 0x8000000. | |
docs/clang-tidy/checks/misc-suspicious-missing-comma.rst | ||
48 | s/represents minimal/specifying the minimum | |
53 | represents->specifying 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 | s/represents maximal/specifying the maximum | |
docs/clang-tidy/checks/misc-suspicious-string-compare.rst | ||
55 | 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 | s/represents/specifying | |
docs/clang-tidy/checks/modernize-replace-auto-ptr.rst | ||
78 | s/represents/specifying | |
docs/clang-tidy/checks/performance-for-range-copy.rst | ||
26 | 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 | s/represents/specifying |
docs/clang-tidy/checks/google-build-namespaces.rst | ||
---|---|---|
21 | 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 | ||
156 | s/bigger than/greater than | |
docs/clang-tidy/checks/performance-for-range-copy.rst | ||
26 | Should quote auto with backtics, I suppose. |
Fix more comments.
docs/clang-tidy/checks/google-build-namespaces.rst | ||
---|---|---|
21 | Aha, sorry, I misunderstood your words here. Yeah, we should add trailing , for the non-extension headers. |
docs/clang-tidy/checks/google-build-namespaces.rst | ||
---|---|---|
21 | Okay, that make sense to me then. This change should also be applied to other places that have the extension list. |
docs/clang-tidy/checks/google-build-namespaces.rst | ||
---|---|---|
21 | All related documents have been updated now. |
s/represents/specifying