Earlier (IIRC in March) we did an internal test of the check and the following results were obtained. The results are kinda weird, to say the least. Numbers are after CodeChecker has done a serverside uniqueing (most likely things such as "if X in H.h is reported in the compilation/analysis of A.cpp and B.cpp then store X only once") of the bugs.

@vabridgers Please take a look at this, as per off-list discussion, in relation to D142822.

In D142822#4095896, @DavidSpickett wrote:

/home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:14:18: error: reference to 'std' is ambiguous
void f(str<char, std::char_traits<char>> &s) {
                 ^
/home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:7:11: note: candidate found by name lookup is 'std'
namespace std {
          ^

In D91000#4099773, @dyung wrote:

To XFAIL the test, grep for lines with "XFAIL" and "ps4" and you should find some examples. It was recently changed how it worked lately.

In D91000#4099669, @dyung wrote:

-DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4

Hopefully this can help you to reproduce the problem.

Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't find any information what x86_64-sie is...

Alright, right now, I have no meaningful idea why this failure appears, locally I'm trying every test command as they appear in the test, and all the tests are passing. It's as if on that system the whole Annex K support would not be allowed. Locally I added a few debug prints and I'm getting the right answers for "Whether Annex K is allowed?".

Oddly enough, one of the buildbots caught a not matching test that was working locally... on it.

Alright, I've done a full reanalysis after a rebase. Overhead is not meaningfully measurable, even for complex TUs such as LLVM. The check is viable in C++ code as it finds cases (such as the one described in LLVM, but also Bitcoin is a primarily C++ project), so I won't rework the check to disable it in C++ mode explicitly. It seems the name lookup is implemented pretty well, helped by the fact that the names to match are short. No crashes had been observed in the test projects; let's hope it stays the same way; the matchers themselves are simple enough. The Annex K. matcher is only registered if in >= C11 mode.
I've also gone through the discussion earlier, and it looks to me as if all the concerns were either resolved or made obsolete due to the evolution of the check.

Alright, the tests came back from the CI positively and there were no crashes on like 15 projects (about 50:50 C and C++). There are some nice results:

Tentative LGTM, will still need to run the "usual test projects" in the CI for confirmation. What's missing from this patch are the .rst files for the CERT-specific aliases of the check, but those are trivial and we can "add them in post" anyway.

In D91000#4020891, @futogergely wrote:

What we could do is:

1. add a new checker option to decide if we suggest replacements from AnnexK. We could avoid registering matchers this way, but I don't really like this, having an option for something we could decide from the defined macros.
2. As a TODO, we could make possible to register checkers AFTER the preprocessor is executed. I have not looked into this, so I don't really know if it is possible at all in the current architecture.

In D91000#3861942, @aaron.ballman wrote:

My concern with that approach was that we pay the full expense of doing the matches only get get into the check() function to bail out on all the Annex K functions, but now that there are replacements outside of Annex K, I don't see a way around paying that expense, so I think my concern has been addressed as well as it could have been.

In D91000#3770071, @whisperity wrote:

In D91000#3197851, @aaron.ballman wrote:

In terms of whether we should expose the check in C++: I think we shouldn't. [...]

I think we should probably also not enable the check when the user compiles in C99 or earlier mode, because there is no Annex K available to provide replacement functions.

@aaron.ballman I think the current version of the check satisfies these conditions. It seems the check will run for every TU, but in case there is no alternative the check could suggest, it will do nothing:

if (!ReplacementFunctionName)
  return;

Is this good enough? This seems more future-proof than juggling the LangOpts instance in yet another member function.

I agree with @martong that $ = realloc($, ...) is indicative of something going wrong, even if the developer has saved the original value somewhere. Are we trying to catch this with Tidy specifically for this reason (instead of CSA's stdlib checker, or some taint-related mechanism?). However, @steakhal has some merit in saying that developers might prioritise the original over the copy, even if they made a copy. I think an easy solution from a documentation perspective is to have a test for this at the bottom of the test file. And document that we cannot (for one reason or the other) catch this supposed solution, but if you have made a copy already(!), then you might as well could have done void* q = realloc(p, ...) anyway! Having this documented leaves at least some paths open for us to fix false positives later, if they become a tremendous problem. (I have a gut feeling that they will not, that much.)

Generally LGTM. Please revisit the documentation and let's fix the style, and then we can land.

Sorry, I'm not sure how I missed it. Been on vacations and such... either way, I think documenting this is a good practice.

In D131319#3708667, @bzcheeseman wrote:

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

I got here from D131319, but I am not sure why this change is only touching comments but not the hand-written documentation. Is this CastInfo an internal detail that should not be publicly documented to the everyday documentation reader? Are the old-style "classof" method and enums still applicable?

Alright, I think we should have this in and let C++17 things to be future work. Please see the inline comment, but otherwise this should be good enough. Can always improve in a future version. 😉

In D124447#3651074, @whisperity wrote:

I will attempt to write a concise response to this today (and tomorrow)! Sorry, I was away travelling to and doing post-action bureaucracy of conferences the past few weeks.

In D124447#3645197, @sammccall wrote:

In D124447#3608747, @aaron.ballman wrote:

In general, I think this is looking pretty close to good. Whether clang-tidy should get this functionality in this form or not is a bit less clear to me. *I* think it's helpful functionality and the current approach is reasonable, but I think it might be worthwhile to have a community RFC to see if others agree. CC @alexfh in case he wants to make a code owner decision instead.

+1 to a need for the RFC here, there are ecosystem questions that I think should be addressed first but aren't in scope of this patch.

LGTM. Thanks!

(Please ensure a more appropriate commit message that actually mentions the check when committing, @steakhal.)

Because of the stability issues related to getName()-like constructs I'm putting a temporary ❌ on this (so it doesn't show up as faux accept). However, I have to emphasise that I do like the idea of the check!

Thank you!

The context of the diff is missing. Please re-run the diff making with -U9999999999999999999.

In D124446#3521619, @whisperity wrote:

@aaron.ballman [...] I think I can put in a measurement job [...]

In D125771#3519606, @LegalizeAdulthood wrote:

I thought there wasn't any support for validating fixits applied to header files?

In D125383#3509084, @aaron.ballman wrote:

Then I think the only thing missing here are updates to https://github.com/llvm/llvm-project/blob/main/clang/lib/ASTMatchers/Dynamic/Registry.cpp.

• Added to ASTMatchers/Registry.cpp
• Updated with release notes

In D125383#3508960, @aaron.ballman wrote:

Do you expect to use this matcher in a new check in the immediate future?

(Also: this is a fix to an issue of your own finding and not something that was reported on Bugzilla? Just to make sure we cross-reference and close tickets.)

LGTM. Some typos inline. Also I think this is a significant enough bug-fix that it warrants an entry in the Release Notes under Changes to existing checks.

In D124447#3506536, @whisperity wrote:

In D124447#3493996, @whisperity wrote:

In D124447#3493446, @aaron.ballman wrote:

precommit CI is showing a fair amount of failures that I believe are related to your patch.

I have no idea what is causing the CI issues, because all the CI issues are related to unit test libraries removing const from fixits and such(??) which this patch (or any in the current patch set) doesn't even touch, at all. I did run the test targets locally... it's likely that simply the rebase and the push happened against an unclean/breaking main branch...

Rebase & rerun against a current main branch produces the issues locally for me, too. So far I have no idea what might be causing this, but I'll keep on digging. However, let's continue with discussing the general approach meanwhile. 🙂

@njames93 What do you think about the current approach? It will under-approximate the problem-inducing node set but at least cover what we know about C++ now.

Just one question if you could try this out for me: what happens if you run clang-tidy a.c b.c (two TUs in the invocation) where one of them (preferably the later one, i.e. b.c) does NOT have Annex K enabled? I believe the cached IsAnnexKAvailable (like any other TU-specific state of the check instance) should be invalidated/cleared in an overridden void onStartTranslationUnit() function.

In D124447#3493996, @whisperity wrote:

In D124447#3493446, @aaron.ballman wrote:

precommit CI is showing a fair amount of failures that I believe are related to your patch.

I have no idea what is causing the CI issues, because all the CI issues are related to unit test libraries removing const from fixits and such(??) which this patch (or any in the current patch set) doesn't even touch, at all. I did run the test targets locally... it's likely that simply the rebase and the push happened against an unclean/breaking main branch...

In D124447#3493446, @aaron.ballman wrote:

precommit CI is showing a fair amount of failures that I believe are related to your patch.

This is unrelated to Clang-Tidy, the change affects the LLVM ADT library.

D'oh! I made a mistake when copying the URL and accidentally associated the commit with D12306 instead of D123065... Anyhow, this was committed in rGb1f1688e90b22dedc829f5abc9a912f18c034fbc.

In D123065#3474107, @bernhardmgruber wrote:

Ping.

Can someone merge this please for me? Thank you!

Regarding FixIts... FixIts are implemented in the "Diagnostic" library, which is non-specific to neither Clang-Tidy nor Sema whatsoever, they use the same infrastructure under the hood. Why the apply logic in CSA might do the same FixIt multiple times is beyond me, but I know that both clang-apply-replacements and clang-tidy go to length to ensure that in case multiple checkers report to the same location with potentially conflicting FixIts, then none gets applied, because applying all of them would result in ridiculously broken source code. They internally become an object in the clang::tooling namespace which is implemented as a core Clang library. The relevant entrypoint to this logic, at least in Clang-Tidy, should be this one: http://github.com/llvm/llvm-project/blob/8f9dd5e608c0ac201ab682ccc89ac3be2dfd0d29/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L115-L134

After adding improvements to the documentation, I think this will be good to go, and thank you! Perhaps just for a safety measure you could run it on a few projects (LLVM itself?) to ensure we didn't miss a case where it might magically crash, but I wonder how many specifically "C++14" projects will use signal handlers in the first place.

[NFC] Fix the test.

@aaron.ballman Alright, I think this can go. The ReleaseNotes.rst needs a rebase anyway.

(Side note: you should avoid the list-expansion syntax in URLs because browsers do not understand them and result in links that are not leading anywhere.)

Alright, I think this is good to go. I like that it makes it clear that the called function is coming from some external source (system header or otherwise).

In D121214#3369871, @steakhal wrote:

Drop the alias-related changes and preserve the note in the bugprone-shared-ptr-array-mismatch.rst stating this relationship with the cert rule?
If we do it like that, then this will be again NFC.

I have some concerns about this. While it is now clear to me that the partialness refers to partial coverage of the guideline rule, it is indeed very, very partial. MEM51-CPP as of now lists 9 cases of non-compliant examples, from which std::shared_ptr<T> = new S[8]; is just one. bugprone-shared-ptr-array-mismatch (D117306) in itself is a check that inherits from a "more generic" smart pointer check.

Is "partial aliasing" a new notion? Or... is the alias not partial, but the coverage?

In D120555#3345707, @njames93 wrote:

If you backport, the release notes change on trunk should then be reverted.