LGTM, chatted with steveire on IRC: blocking this for a better cmake based solution makes no sense. So your free to commit :)

LGTM with the doc-nit.

One thing: Could you please check the utility-scripts in clang-tidy that create new checks, if they need adjustments? Not sure if we have something in there that relies on the old structure.

You also get into trouble because there are many header files that it add LLVM_NODISCARD to but they don't include "Compiler.h" where LLVM is defined (so you end up with lots of errors)
3>C:\llvm\include\llvm/ADT/iterator_range.h(46): error C3646: 'IteratorT': unknown override specifier (compiling source file C:\llvm\tools\clang\utils\TableGen\ClangCommentHTMLNamedCharacterReferenceEmitter.cpp)
I'm wondering if there is anything I can add to the checker, to check to see if the macro being used for the ReplacementString is defined "in scope"

Did you rebase the check onto the new master with your refactorings in?

In D55595#1328263, @steveire wrote:

Can you say where else it is common in LLVM? I'm curious. Maybe those places could be changed too.

Sounds like we might not correctly set the parent map for CXXConstructorDecl then?

Neat script, I think we could even plug that into arc diff. AFAIK it can run linters

In D55595#1328154, @steveire wrote:

FYI, CMake target property INTERFACE_SOURCES is designed to make this easy.

For each module you would generate a file containing

extern volatile int ${MODULE_NAME}ModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED ${MODULE_NAME}ModuleAnchorDestination =
    ${MODULE_NAME}ModuleAnchorSource;

and then put that generated file in the INTERFACE_SOURCES of each module.

target_sources(${MODULE_NAME} INTERFACE ${THE_GENERATED_FILE}.cpp)

Then, you don't need to maintain it in C++ like this. It is DRY because the target_link_libraries entry for the library is what causes the symbol to be used.

In D55594#1328061, @MyDeveloperDay wrote:

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)

LGTM

In D55541#1326868, @lebedev.ri wrote:

In D55541#1326867, @JonasToth wrote:

Please remember to upload your patches with full context (i can highly recommend using arc, see https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch and https://llvm.org/docs/Phabricator.html for the relevant command).

And to add to that, please do set the correct "Repo" field, correct tag if appropriate (clang-tools-extra), and subscribe the appropriate -commits list (in this case, cfe-commits.)

Please remember to upload your patches with full context (i can highly recommend using arc, see https://llvm.org/docs/DeveloperPolicy.html#making-and-submitting-a-patch and https://llvm.org/docs/Phabricator.html for the relevant command).

LGTM

LGTM

See PR39949 as well, as it seems to trigger the same or a similar problem.
@ioeric and @klimek maybe could give an opinion, too?

In D55508#1325960, @Eugene.Zelenko wrote:

By the word, will be good idea to have script which check alphabetical order and use it during build. Sometimes alphabetical order may be violated after merge with trunk.

In D55508#1325834, @MyDeveloperDay wrote:

In D55508#1325758, @JonasToth wrote:

LGTM. Do you have commit access?

I do not I'm afraid

LGTM. Do you have commit access?

i think the idea is good to have more tools that do style checks for us.
Plugging them into the normal test-suite is maybe a good idea, as otherwise everyone forgets to use them ;)

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

Hi MyDeveloperDay,

Please upload with full context.

Please upload with full context.

Please upload the patch with full context.

Hi Sam,

In D55245#1320546, @hwright wrote:

Ping.

I assume I've got the right reviewers here, but I've also been sending a bunch of stuff your way lately, so if I'm overwhelming review capacity, please let me know.

I had to revert this patch because it broke (at least one) buildbot with an assertion-failure (http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40436/steps/test/logs/stdio)

Committed, Thank you for the patch! Was there a bug-report for this issue? If yes can you please close it/reference?

A quick, tangentially related idea: Would it be useful to implement multiline nolint sections (e.g. //BEGINNOLINT – //ENDNOLINT or something similar)? This would be helpful for using clang-tidy on projects that contain some automatically generated fragments.

int8 ? Did you mean int8_t or am I missing somthing ?

I had to revert and recommitted in rCTE348169. std::unordered_map<enum class Something, ...> does not work, as std::hash is not specialized for it. This behaviour seems to work for some compilers, but some not. I had to google myself a bit for the best solution, but now I specialized the hash-function to std::hash<std::int8>() in unordered_set which worked for me locally and was suggested. Lets see ;)

Hi russell

from my side no objections, mailing list did not react AFAIK (just in case your waiting for me until you recommit).

In D54757#1316899, @donat.nagy wrote:

I applied this check to the llvm + clang codebase, and I was surprised to see that it produced about 600 results (for comparison, the clang-tidy checks which are enabled by default produce approximately 6000 results together). I didn't go through the whole list, but after examining a few dozen random reports it seems that most of these are true positives: relatively short branches (usually one or two lines of code) are repeated for no obvious reason. I would guess that most of these fall into the "correct but too verbose" case, because otherwise the tests would have caught the problem, but I didn't try to understand their context.

I have seen a few false positives related to preprocessor trickery: when an .inc file is included to create a huge switch, sometimes it will become a huge switch with lots of identical branches. There were also some situations where the check reported identical branches which are annotated with different comments; I don't know if this should be considered a false positive.

If this is acceptable, then I would be grateful if someone would commit this patch for me as I don't have commit rights.

In D55044#1312438, @lebedev.ri wrote:

Thanks for working on this.
Semi-duplicate of D50852 Please see discussion there.
It should not be an abseil-specific check, the prefix (std,abseil), and the function (make_unique) should be a config option, and it should likely be a part of modernize-use-auto.

In D54737#1314946, @hwright wrote:

Oh, and thanks for taking the time to review this. :)

Wiring out the lexical comparison and AST based comparison sounds like an interesting idea, however IMHO such a setting is too coarse-grained on the file level. My guess would be that it depends from expression (fragment) to expression fragment how you want to compare them: for macros lexical comparison is better, for arithmetic expressions with many parentheses AST based recursive comparison may be a better fit (as implemented also in this checker).

In D55125#1314741, @Szelethus wrote:

@JonasToth this is the Lexer based expression equality check I talked about in D54757#1311516. The point of this patch is that the definition is a macro sure could be build dependent, but the macro name is not, so it wouldn't warn on the case I showed. What do you think?

Only the test and your opinion on the Optional thing, If you want to keep it that way its fine.

LGTM, could you please run this check over real world code and check that there are no obvious false positives?

In D54757#1311516, @Szelethus wrote:

In D54757#1311468, @donat.nagy wrote:

Macros:

The current implementation of the check only looks at the preprocessed code, therefore it detects the situations where the duplication is created by macros. I added some tests to highlight this behavior.

The only option I see to detect macro related errors is to use a Lexer, and compare the tokens one by one, but that might be overkill, and I'm aware of a check in the works that already implements that. I think it's perfectly fine not to cover those cases, but you should definitely document it somewhere, preferably in the rst file.

The issues seems to be resolved for me as well, i will post this patch to the mailing list and ask the other guy if his build is fine, too.

LG from my side, only the style nits left.
other reviewers are invited to take a look too :)

Thank you for the fix! I will try this version as soon as I am at home (~2 hours from now) and report back if the issues I had are gone.

Hi Jonas, I am sorry for the breakage.

Revert in https://reviews.llvm.org/rL347676