• be explicit about the State enumerator values

all comments resolved. I will land this now.

• [Refactor] move support classes into the analyzer
• [Refactor] move bigger methods into implementation file
• minor adjustments

ping :)

In D57662#1392509, @MyDeveloperDay wrote:

Just a question.. If clang tidy is running with -fix in parallel, what stops each clang-tidy invocation altering a common header at the same time?

LGTM

Good idea :)

• [Refactor] move support classes into the analyzer
• [Refactor] move bigger methods into implementation file

ping

Landed in rL353389. Thank you for the patch!

How are the semantics for enum class in this case?

@oleg.smolsky I can commit for you. Please note, that LLVM changed its license recently and this patch will land under the new license.
Please confirm that this is ok for you (more info on the license see https://llvm.org/docs/DeveloperPolicy.html#relicensing). I will wait so long :)

Definitly! I just asked because it would be better to mention it in the message ;)
Do you have commit rights?

LGTM, was there a bug report for that issue?

LGTM, please close the bug report and notify for the fix with the request to check in the real-project if it actually fixes the issue. Thanks :)

LGTM. Committing both revision together would be good :)

• Merge branch 'master' into feature-introduce-noexcept

• [Misc] rewrite parts of a comment that were unprecise.

In D57100#1377687, @baloghadamsoftware wrote:

In D57100#1373440, @JonasToth wrote:

@baloghadamsoftware Do you see any problems with the refactoring, especially the new FunctionDecl*-Caching is not exactly a refactoring. If you have any thoughts on that would be great to hear!

Yes, it looks a good improvement, I expect another huge speedup. (We had a previous huge speedup after I optimized the matcher expression in 7.0.1 because in 7.0.0 it seemed to hang, it was really bad).

In D57100#1373430, @baloghadamsoftware wrote:

Hello! I am glad to see that this check gets improved by the community. I also think a "modernize" check which marks functions with noexcept is also useful.

ping.

LGTM

LGTM with the few language nits.
The new matchers look better and are more matcher style, gj :)

• last nit, remove the disabled in hexadecimal

• Revert "add REQUIRES: <arch> for the readability unit-tests where necessary" as its not necessary because only the frontend is run

• add REQUIRES: <arch> for the readability unit-tests where necessary

In D55523#1371336, @MyDeveloperDay wrote:

In D55523#1371297, @JonasToth wrote:

The question now is: should we split the rst linting from other aspects? Imho yes and put the .rst-linting into LLVM space. We can still have a local .arclint-file to force only the CTE ppl into using it.

I'd keep the rst linting separate from the other linters, the "include" part will ensure only the .rst files in the review will be checked.

The monorepo-vs-non-monorepo is still interesting as we can specify both correct paths, can we?

No we can only have one path, which would need it to be different in monorepo-vs-non-monorepo, but this would be true for the clang-format linter suggested in D49116 too, so that linter will only work for the main llvm repo, (not clang or CTE)

The .arclint file will only work in CTE with the same .arclint file (copied into CTE) if both the .arclint file and the scripts are put into a new utils directory in tools/extra to match that of llvm (and if the clang repo wanted the same again it would need to be duplicate)

The question now is: should we split the rst linting from other aspects? Imho yes and put the .rst-linting into LLVM space. We can still have a local .arclint-file to force only the CTE ppl into using it.
But in principle I would like to have that.
The monorepo-vs-non-monorepo is still interesting as we can specify both correct paths, can we?

@JonasToth we could have a got at making an .arclint configuration that would run the validate_check.py with the --rst option to pass a rst filename

In D49116#1371143, @starsid wrote:

In D49116#1370993, @JonasToth wrote:

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

The revision is up for grabs. Last I checked, the only issue was that I did not have a Windows machine to test the change.

it is very noisy (in other projects too) and i would prefer a different warning policy for that check as well (e.g. one warning per class, not for every field it warns against).

Drive-by +1, really would like to see clang-format and linters integrated for automatic nitting.

In D55523#1370967, @MyDeveloperDay wrote:

Phabrciator can use Herald rules to react to a change in a revision

If that is a gobal rule it can run a Harbormaster build plan, I can't tell if this will work (because I don't have permission on the LLVM phabricator) but someone has already set up a arc lint + arc unit build plan

it may be possible to use that, if thats the case we may just be able to set up the validation of .rst files are as a arc linter in .arcconfig (without needing people to use arc to submit the reviews)

alternatively Harbormaster can make an HTTP request to a CI system, passing information about the review PHID, its then actually very simple json and wget or curl type calls to talk back to Phabricator to insert review comments

see https://reviews.llvm.org/conduit/method/differential.createinline/

So you could imagine, someone makes a review, and a CI system pulls the contents, runs some simple checks (like are you clang-formatted) then inserts a review comment back into the reivew..

This means you don't really need to hack in php (although I've customized our own phabricator its very easy as it has an extensibility mechanism)

Phabricator is very powerful, we are only really scratching the surface.. take a look here at some of the things you can do...

https://reviews.llvm.org/conduit/

To be honest it would probably best if we made contact with whoever is the administrator of Phabricator (do we know who that is?), because there are some very simple things that can be added to make life a little easier.. (for example like using the bugtraq.logregex and bugtraq.url settings to automatically turn Bugzilla PR numbers in messages into hyperlinks) (http://www.howtobuildsoftware.com/index.php/how-do/bFAT/issue-tracking-phabricator-link-to-issue-page-from-within-phabricator)

In D55523#1369784, @MyDeveloperDay wrote:

In D55523#1369634, @Eugene.Zelenko wrote:

In D55523#1369424, @alexfh wrote:

A few questions:

1. why do we want to lint just clang-tidy docs and not all LLVM's .rst files?

Proposal was made on llvm-dev, but it didn't attract interest.

Maybe I'll just put the script over on github in a clang-utils project so its easier to download...and then Abandon this review here....then we can always point people at it if they find it useful.

• rebase to latest patch

• revert doc change as well

Reverted the changes. Maybe the comma thingie will just stay as is ;)

• revert the option configuration change for comma/semicolon

In D57108#1369161, @lebedev.ri wrote:

Hmm, i think this diff is incorrect. Doesn't it also include the changes from D57100 ?

• rebase to current refactoring

• add license to exceptionanalyzer

• [Fix] make class name correct

• adjust doc to say semicolon separated list instead of comma

In D57100#1367813, @lebedev.ri wrote:

Two passing-by remarks:

1. I would *love* for this check to produce less cryptic reports :) It currently does not output any details whatsoever. It's really unhelpful.

In D56160#1367074, @bernhardmgruber wrote:

Thank you again @JonasToth for all your valueable input! I could almost successfully run my check on the llvm/lib subfolder. I created a compilation database from within Visual Studio using an extension called SourceTrail. One of the issues was the following:

ping :)

• use only the small reproducer

In D56926#1363648, @MyDeveloperDay wrote:

the closest I can see is

https://www.perforce.com/resources/qac/high-integrity-cpp-coding-standard#statements

which take you to section 6 of the standard, but I see no id or name tags to get you to 6.3.1

very good idea. This reminds me, that i wanted to fix the hicpp links, as they restructured their website. thanks ;)
Changes LGTM!

In D56610#1363461, @ilya-biryukov wrote:

In D56610#1363408, @JonasToth wrote:

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

No, this action is not about adding a type qualifier (like const or volatile), it adds a namespace qualifier, e.g.

using namespace std;

vector<int> foo; // --> std::vector<int> foo;

Moreover, it's just an example to illustrate how one could write a simple action like that in clangd.

Is this for something like add const?
If yes, there is clang-tidy effort on that, see https://reviews.llvm.org/D54943 and https://reviews.llvm.org/D54395 for a similar effort. Would be best to share the code instead of reinventing it :)

In D54141#1362924, @MyDeveloperDay wrote:

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

Do we need a llvm-elide-braces-for-small-statements?

This would make a great pre-review check