LGTM

the closest I can see is

Abandoning revision on the basis of perhaps going for a more clang-tidy only directed error message.

In D56901#1363040, @alexfh wrote:

Where does this problem come from?

LLVM is very chatty as well, I don't consider LLVM to be a bad code-base. Take readability-braces-around-statements for example.

Abandoning...recreated as D56901: [Support] Fix Windows Command Shell Command line parsing for quoted arguments

In D56424#1359227, @karepker wrote:

In D56424#1357484, @MyDeveloperDay wrote:

In D56424#1357481, @lebedev.ri wrote:

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

Hi all, ping on this patch. I've addressed all comments to the best of my ability. Is there anything outstanding that needs to be changed?

Round about this time of a review we normally hear @JonasToth asking if you've run this on a large C++ code base like LLVM (with fix-its), and seen if the project still builds afterwards..might be worth doing that ahead of time if you haven't done so already

From docs: This check does not propose any fixes..

Thats a great suggestion for the future then.... transform

TEST(TestCase_Name, Test_Name) {}

into

TEST(TestCaseName, TestName) {}

I considered doing this for the check, but decided against it based on the cases in which I've seen underscores in use. I've seen a few cases in the style of this:

SuccessfullyWrites_InfiniteDeadline
SuccessfullyWrites_DefaultDeadline

changing these to:

SuccessfullyWritesInfiniteDeadline
SuccessfullyWritesDefaultDeadline

has a subtle difference to the reader. In the first case, underscore functions like "with", whereas in the fix it sounds like the test is for writing the deadline.

While removing the underscore does seem to work for a large portion of the cases, based on the cases like that above, I didn't think it was appropriate to make these modifications.

Please let me know what you think.

In D56563#1358149, @JonasToth wrote:

Addressing review comment

In D56424#1357481, @lebedev.ri wrote:

In D56424#1357471, @MyDeveloperDay wrote:

In D56424#1356959, @karepker wrote:

Hi all, ping on this patch. I've addressed all comments to the best of my ability. Is there anything outstanding that needs to be changed?

Round about this time of a review we normally hear @JonasToth asking if you've run this on a large C++ code base like LLVM (with fix-its), and seen if the project still builds afterwards..might be worth doing that ahead of time if you haven't done so already

From docs: This check does not propose any fixes..

In D55433#1351707, @JonasToth wrote:

I do not have commit rights. I'm not sure what constitutes someone who can commit, but let me contribute a little more before taking that step, I have another idea for a checker I'd like to try after this one, I just wanted to get one under my belt first.

See this: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access

I will commit for you then. More patches always welcome ;)

In D56424#1356959, @karepker wrote:

Hi all, ping on this patch. I've addressed all comments to the best of my ability. Is there anything outstanding that needs to be changed?

Fix review comments

I think will be good idea to create generic Case/Prefix/Suffix description to reduce size of documentation.

Address review comments

• clang-format the code examples
• replace "ensure" with "check"

nit's come from running running validate_check.py on the rst file from D55523: [clang-tidy] Linting .rst documentation

Addressing some formatting review comments

• add ability to run with --rst to perform on a single .rst file (useful for validating docs not in clang-tidy)
• add ability to ignore the 80 character rulles (with -no-maximize), (some of the other documentation has lots of this violations and we might want to fix the obvious checks first)

Thanks @JonasToth for the help (again) and sorry to break the build like that.

I'd like to understand what went wrong here? is it just the missing tabs then 80 characters?

@JonasToth thanks...

In a later change it might be worth mentioning...

! In D56482#1350874, @kristina wrote:

I only added a section, all the warnings are from existing bits below. Not sure if it's a good idea to reformat it as part of the diff as it will make it harder to see the added section. Can do it afterwards but that's what's currently being used to generate documentation.

In D55433#1350999, @JonasToth wrote:

LGTM!
You verified that your fixes, fix the issues in LLVM? But it looks good to go.

$ ./validate_check.py --rst ../../../../../docs/Phabricator.rst
Checking ..\..\..\..\..\docs\Phabricator.rst...
warning: line 97 multiple blank lines
warning: line 166 is in excess of 80 characters (82) : as it will retrieve reviewers, the `Differential Revision`, etc from the review
...
warning: line 175 contains double spaces
warning: line 188 is in excess of 80 characters (97) : The first command will take the latest version of the reviewed patch and apply it to the working
...
warning: line 194 is in excess of 80 characters (111) : This presumes that the git repository has been configured as described in :ref:developers-work-with-git-svn.
...
warning: line 203 multiple blank lines
warning: line 205 is in excess of 80 characters (84) : current `master and will create a commit corresponding to D<Revision>` with a
...
warning: line 208 is in excess of 80 characters (85) : Check you are happy with the commit message and amend it if necessary. Now switch to
...
warning: line 218 multiple blank lines
warning: line 219 multiple blank lines

In D55433#1349709, @JonasToth wrote:

No problem, thats why we test on real projects first, because there is always something hidden in C++ :)

Is there a test for the lambdas?

Address review comments

• add more lambda tests
• add nested lambda test
• remove hasParent() call, (seems isConversionOperator()) seems to detect the CXXMethodsDecls within a lambda that were causing [[nodiscard]] to be added before the []{... )

In D55433#1349709, @JonasToth wrote:

No problem, thats why we test on real projects first, because there is always something hidden in C++ :)

Is there a test for the lambdas?

In D55433#1349709, @JonasToth wrote:

No problem, thats why we test on real projects first, because there is always something hidden in C++ :)

Is there a test for the lambdas?

Sorry to add another revision but, rerunning no-discard checker on LLVM code base threw up other [[nodiscard]] placements which whilst they MAY be correct, I don't think we should handle in this first pass.

• conversion functions e.g. operator bool()
• lambdas
• add tests to validate both

libprotobuf still builds cleanly with just the one expected warning..despite adding over 500 [[nodiscards]]

Update with view comments from @JonasToth

• remove unnecessary static functions
• move checks into matchers
• remove duplicated diag call

Address review comments

• remove using strings to locate std::function and boost::function arguments using matcher with `hasAnyName()` instead
• add tests to check for const, const & for above

let me look further into that, it works for std::function but not for const std::function or std::function &, const std::function &

Rebase

• update to address most review comments from @JonasToth
• simplify matcher (with less unless)
• remove the need to look for "type-parameter-" string

• Rebase
• Add a couple of extra test cases to ensure we don't add the NO_DISCARD macro when a clang or gcc attribute is present
• Ping to ask code owners to review and possibly commit if happy.

Remove unused matcher variable

In D55433#1334150, @curdeius wrote:

LGTM.
Any ideas for improvement, @JonasToth?
I'd rather have it merged and improve on it later if there are ideas on how to do it better.

Fix review comments

• Fix minor spelling mistakes
• add additional boost::function unit test

-Refactor the checker to remove the llvm::any_of by using a QualType ast-matcher with hasAnyParameter(hasType(x))

• i.e. ...unless(hasAnyParameter(hasType(isNonConstReferenceOrPointer())))...

• Rebase
• Reduce false-positives by not adding [[nodiscard]] to functions if the class has mutable fields (removed as known issues) and add tests

Fix review comments

• minor changes to code comments
• add more unit tests for typedef'd template types

In D55523#1328535, @Eugene.Zelenko wrote:

I think will be good idea to rename script to validate_documentation.py. Or may be lint?

Add additional constraints on member functions

• do not add [[nodiscard]] to variadic function
• do not add [[nodiscard]] to std::function templates argument functions
• do not add [[nodiscard]] to functions returning template types
• add unit test to ensure above

Addressing review comments

• don't add [[nodiscard]] onto function taking template arguments
• add more unit tests to cover other discard possibilities [[clang::warn_unused_result]],[[gcc::warn_unused_result]]
• improve the documentation to help explain when the [[nodiscard]] will/will not be applied

unsigned BlockOrCode = 0;
llvm::ErrorOr<Cursor> Res = skipUntilRecordOrBlock(Stream, BlockOrCode);
if (!Res)
  Res.getError();

AFAIK llvm::Error must be consumed because if it goes out of scope unhandled it will assert. Not sure how to handle that.

Address review comments

I thought I would just correct readability-* and zircon-* to get some feedback if you think this is improving the documentation the way you think it should, if this is OK I can follow up with the other checks (not sure if they are better in separate commits or all together)

In D55523#1326963, @Eugene.Zelenko wrote:

In D55523#1326848, @MyDeveloperDay wrote:

In D55523#1326082, @Eugene.Zelenko wrote:

Other common problems which will be great to detect with this script:

• Ensure same length of headers titles and markup.

Could you explain what you mean by this? I'm assuming you mean that when you see a title with --- or === or ^^^ then you expect the line about to be the exact same length, is that correct?

Example
-------

Yes, you understood correctly.

Reduce false positives

Example output from running with validate_check.py --check-all

Add capability to check all the checks in one go (suitable for a build step)

in looking closer at an example of the output..

Added new checks to help reduce review comments in documentation

• check for unnecessary empty lines at the end of the rst files
• check for double spaces
• check for extraneous multiple blank lines
• check for extraneous trailing whitespace
• ensure markup title and markup characters --- === ^^^ match in lengh
• exclude lines containing <clang-tidy
• reformat message to look like a compiler warning

In D55523#1326082, @Eugene.Zelenko wrote:

Other common problems which will be great to detect with this script:

In D55523#1326082, @Eugene.Zelenko wrote:

Other common problems which will be great to detect with this script:

• Double spaces
• Empty lines at end of file
• Ensure same length of headers titles and markup.

I wanted to leave a comment regarding running the [[nodiscard]] checker over all of clang... as requested by @JonasToth, @lebedev.ri and @Szelethus

Resolving some (not all yet) review comments, and looking for help on template parameter exclusion

• add additional template argument tests
• add additional clang-warn-unused-result tests
• add additional gcc-warn-unused-result tests
• exclude template parameters
• improve nodiscard wordage in documentation regarding conditions of checker

Addressing review comments

• grammatical errors in documentation and comments
• prevent adding [[nodiscard]] to macros
• clean up list.rst

In D55508#1325758, @JonasToth wrote:

LGTM. Do you have commit access?

In D55508#1325670, @JonasToth wrote:

I think this patch is fine, AFAIK these utility scripts are not tested directly but are just adjusted if they dont work as expected :)

Did you test it with a fake new-check? If it does what we expect its fine :)

@curdeius Thanks, I don't have commit access so I'm happy wait for a CODE_OWNER, they could likely have more input.

Fix review comments

Addressing review comments,

• additional unit tests for no ReplacementString and C++ 11 case
• more expressive hasNonConstReferenceOrPointerArguments matcher
• minor documentation typos and spacing

Update the documentation to utilize 80 character lines

In D55508#1325316, @Eugene.Zelenko wrote:

Thank you for this fix!