Have you run this check over any large code bases to see just how often it triggers and whether the diagnostics are false positives or true positives? I have a sneaking suspicion that this will yield a *ton* of false positives and probably not a whole lot of true positives. For starters, the issue isn't really a bugprone one -- the recommendation is about the behavior when compiling for different architectures or with different compilers, so this is more of a portability issue. It may make sense to move this into the portability module instead. However, the other issue is: a lot of CERT recommendations are recommendations rather than rules because it's impossible to determine programmer intent from the source code. If the user is intentionally using these literals, how should they silence this diagnostic?

In D83717#2364187, @gamesh411 wrote:

In D83717#2279263, @aaron.ballman wrote:

One of the concerns I have with this not being a cfg-only check is that most of the bad situations are not going to be caught by the clang-tidy version of the check.

...

Have you run this check over any large code bases to see if it currently catches any true positive diagnostics?

I have tried llvm, tmux, curl and tried codesearch.com to look for other sources containing atexit, but no clang-tidy results were found for this check (this is partly because it is hard to manually make the project results buildable). So it is hard to see whether this flow-sensitive approach would result in many false positives.

LGTM aside from a request about the note. Thank you for this!

LGTM aside from the point @rjmccall made about the note.

LGTM!

In D89743#2360258, @sammccall wrote:

(while this is useful to you, let's keep discussing, but I'm also happy to stop and land this with your preferred API/semantics - just LMK if changes are needed)

LGTM!

In D90109#2360538, @dsanders11 wrote:

What issues did you run into regarding testing, because I feel like the patch should have test coverage (esp given that color vs ANSI escape codes are a bit of an oddity to reason about)?

I'm unable to create a debug build, due to issues with LLVM and Visual Studio 16.7. There's Google results on the issue, but I don't see any solution and I'm using master and the latest version of Visual Studio. It seems you can (and perhaps should?) run the tests with a Release build, so I've gone ahead and done that.

LGTM!

Adding @klimek as a reviewer since I forgot to do that last time.

In D89743#2359764, @sammccall wrote:

In D89743#2356172, @aaron.ballman wrote:

I was sort of expecting hasAttr() to be deprecated because it is a leaky abstraction (consumers of the API have to know about our internal naming convention for attribute kind enumerations, it makes it harder for use to change the definition of the enumeration, etc.

Sure, SGTM

I'm totally fine if this happens in a follow-up patch (or patches). WDYT?

I definitely don't agree that hasName() has different semantics for attributes from declarations -- both of them are named identifiers, potentially with scope information as part of the name. Making the matcher polymorphic matches my expectations (pun intended).

Fair enough - this isn't really my area, but the differences I was thinking of were:

• the Attr uses a name rather than owning it - it's more like DeclRefExpr than like NamedDecl
• it wasn't clear to me that you'd want the same rules about optional scope handling, though it sounds like you do

In D90244#2357510, @njames93 wrote:

In D90244#2357130, @aaron.ballman wrote:

Was this caused by a performance concern when profiling something? I'm not opposed to the changes, but I do think the original formulation is easier to read.

It's not a huge performance concern, but removing up to 156 malloc calls* for each time we read** or store the style is certainly reason for this.

*Depends on standard library implementations small string optimisation buffer size.
**Happens multiple times as of 4888c9ce.

LGTM aside from a question about a potential assert to add.

In D90180#2357178, @trixirt wrote:

Discussion of change on LKML
https://lkml.org/lkml/2020/10/27/2538

On why the existing clang fixit is not practical.
tl;dr
10,000 changes to look for a treewide fix
The fixit has whitespace issue

LGTM aside from a few request for comments in the test cases.

Was this caused by a performance concern when profiling something? I'm not opposed to the changes, but I do think the original formulation is easier to read.

In D90042#2356265, @flx wrote:

In D90042#2356180, @aaron.ballman wrote:

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

Any reason not to lift enough of the actual definition to be able to reproduce the issue in your test cases? Does the change in definitions break other tests?

I poured over the actual definition and couldn't find any difference wrt the call operator that would explain it. I would also think that:

template <typename T>
void foo(T&& t) {
  std::forward<T>(t).modify();
}

would be a simpler case that should trigger replacement, but it doesn't. Do you have any idea what I could be missing?

This change LGTM, but please wait a few days in case @alexfh (or others) have concerns.

In D54943#2352196, @0x8000-0000 wrote:

Would it be possible to split this review further, into "checking" and "fixing" portions? I understand that fix-it portion is more difficult, and sometimes results in multiple 'const' keywords being added, but for the past year we have used the check as part of regular CI runs to flag where it needs to be added by the engineers and for our use case it works fine that way - so I would prefer to have at least the "report" part as part of upstream Clang12, even if the 'fixup' comes later, due to the complexity of covering all corner cases.

In D90180#2354931, @njames93 wrote:

Is this not already handled by -Wextra-semi. If it isn't I feel like it should be handled there rather than in clang-tidy

In D90109#2352180, @dsanders11 wrote:

Added a few inline comments for clarification.

Note: I was not able to get a debug build and unit tests working on my Windows set up, so this has only been tested manually. I'm not sure if the unit test added in D79477 runs on Windows, but in theory it should be broken on Windows since it expects ANSI escape codes in the output.

In D90042#2350035, @flx wrote:

I should note that I was only able to reproduce the false positive with the actual implementation std::function and not our fake version here.

In D89743#2348487, @sammccall wrote:

In D89743#2342229, @aaron.ballman wrote:

My preference is to add support for hasName() as that's going to be the most common need for matching attributes.

Sounds good (though I ran into issues calling this hasName specifically, see below). There's overlap with hasAttr(Kind) but I think that only handles decls, you can't bind the attribute itself, and it's harder to extend to the other stuff you mentioned.
So maybe hasAttr gets deprecated, or maybe has(attr(hasName("foo"))) is a mouthful for a common case so the shorthand is nice.

In D89988#2354332, @george.burgess.iv wrote:

Doing fun tricks with the CFG sounds like a reasonable next step here, and doing so will likely be way more powerful (+ CPU-consuming, and invite potentially more false-positives). I agree that that probably shouldn't be under -Wall, but I wonder if these trivial cases aren't worth putting there on their own? It's not a perfect parallel, but our split between -Wuninitialized + -Wsometimes-uninitialized comes to mind.

In D89988#2354175, @cjdb wrote:

In D89988#2354128, @aaron.ballman wrote:

In order to avoid false negatives, I think this needs to be flow-sensitive to track values. e.g., I would love for this to catch code like:

void func(bool b) {
  int i = 12;
  int *ip = b ? &i : (int *)malloc(sizeof(int));

  ...

  free(ip); // Should we warn or not?
}

While this example is incredibly contrived, you'll fine real-world code does this sort of path sensitive stuff relatively often (a common example is a function that sometimes returns a value that's been strdup()'ed except for one bad function that returns a string literal).

Yep, this is what I'm considering "phase 2" or "phase 3". Here's another one of note:

char* p = static_cast<char*>(std::malloc(8));
char& r = *p;
std::free(&p); // should warn
std::free(&r); // shouldn't warn

A key intention of D89988 is to get minimal coverage and then expand from there.

In order to avoid false negatives, I think this needs to be flow-sensitive to track values. e.g., I would love for this to catch code like:

void func(bool b) {
  int i = 12;
  int *ip = b ? &i : (int *)malloc(sizeof(int));

LGTM

In D88645#2347725, @Tyker wrote:

In D88645#2347050, @aaron.ballman wrote:

LGTM aside from a request for a comment to be added. Thank you!

do you mean an RFC on llvm-dev/cfe-dev ?

Thank you for this!

In D72553#1815497, @njames93 wrote:

could this do with an alias into performance?

In D89651#2342367, @gbencze wrote:

In D89651#2338266, @njames93 wrote:

Should point out there is already a check for cert-oop57-cpp, added in D72488. Do these play nice with each other or should they perhaps be merged or share code?

I would certainly expect some duplicate warnings with there two checks, but as far as I can tell that check does not currently warn on using memcmp with non-standard-layout types.
I personally would leave the exp42 and flp37 parts of this check as separate, because those are useful in C as well. But maybe it'd be better to move the warning for non-standard-layout types from here to the existing one.

LGTM aside from a request for a comment to be added. Thank you!

LGTM with an extra testing request (that should hopefully just work for you).

LGTM aside from some small nits. You should wait a day or two before landing in case @rnk or other reviewers have further comments.

In D74299#2339994, @AlexanderLanin wrote:

I cannot reproduce the problem and there is just not enough info to go on.

In D85697#2340258, @njames93 wrote:

In D85697#2339055, @aaron.ballman wrote:

I tend to be very skeptical of the value of checks that basically throw out entire usable chunks of the base language because the check is effectively impossible to apply to an existing code base. Have you run the check over large C++ code bases to see just how often the diagnostic triggers and whether there are ways we might want to reduce false-positives that the C++ Core Guidelines haven't considered as part of their enforcement strategy?

I disagree with this mentality, This is following what the core guideline states perfectly. If a codebase doesn't want to adhere to those guidelines, they don't need to run this check. If you think there are shortcomings in the guidelines, raise an issue on their github.

In D89743#2341900, @sammccall wrote:

In D89743#2341779, @aaron.ballman wrote:

This is *awesome*, thank you so much for working on it!

Thanks for the comments - haven't addressed them yet, but wanted to clarify scope first.

One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute"

Yes definitely, I should have mentioned this...

My main goal here was to support it in DynTypedNode, as we have APIs (clangd::SelectionTree) that can only handle nodes that fit there.
But the common test pattern in ASTTypeTraitsTest used matchers, and I figured a basic matcher wasn't hard to add.

In D89638#2341855, @martong wrote:

I was thinking about an alternative approach (5th approach (?), yeah, I think we need an RFC).

What if we had just one attribute with a StringArgument ?
Actually, we already have that with the annotate attribute.
How do you like this?

struct [[clang::annotate("CSA:supress:RefCntblBaseVirtualDtor")]] Derived2
    : RefCntblBase{};

Disadvantages: we must process strings whenever a node has the annotate attr attached, we have to come up with a "DSL".
Advantages: total flexibility.
WDYT?

Adding a few more reviewers in case I've got something wrong.

This is *awesome*, thank you so much for working on it! One question I have is: as it stands, this is not a particularly useful matcher because it can only be used to say "yup, there's an attribute" -- are you planning to extend this functionality so that you can do something like attr(hasName("foo")), or specify the syntax the attribute uses, isImplicit(), etc?

I tend to be very skeptical of the value of checks that basically throw out entire usable chunks of the base language because the check is effectively impossible to apply to an existing code base. Have you run the check over large C++ code bases to see just how often the diagnostic triggers and whether there are ways we might want to reduce false-positives that the C++ Core Guidelines haven't considered as part of their enforcement strategy?

In D72218#2334787, @ffrankies wrote:

Addressed comments by @aaron.ballman regarding the diagnostic warning.

I tried to add a test case for when the filename is kernel.cl, verilog.cl, or vhdl.cl, but that did not work because the test suite appends .tmp.cpp to the end of the test files, and kernel.cl.tmp.cpp is not a restricted filename. If you know of a way to include this test case in the test suite, please let me know.

LGTM aside from a minor nit.

In D74299#2338823, @aaron.ballman wrote:

In D74299#2338772, @AlexanderLanin wrote:

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <alex@lanin.de>

Thanks for review @aaron.ballman!

Happy to do so! I've commit on your behalf in 627c01bee0deb353b3e3e90c1b8d0b6d73464466

In D74299#2338772, @AlexanderLanin wrote:

Could someone commit this as I cannot? Thanks a lot!
Alexander Lanin <alex@lanin.de>

Thanks for review @aaron.ballman!

LGTM!

In D89332#2338319, @njames93 wrote:

Come to think of it, this is a pretty illogical way to solve this problem, just append ::std::function to the AllowedTypes vector in registerMatchers and be do with it. Will require dropping the const Qualifier on AllowedTypes, but aside from that it is a much simpler fix.
The has(Any)Name matcher has logic for skipping inline namespaces.

LGTM aside from a documentation request, but you may want to wait a few days before committing in case Richard or John have opinions.

LGTM aside from some very small nits (feel free to ignore any that don't make sense to you).

Thank you for the continued work on this feature! I'd like to better understand the behavior of fallthrough labels because I think there are two conceptual models we could follow. When a label falls through to another label, should both labels be treated as having the same likelihood or should they be treated as distinct cases? e.g.,

switch (something) {
case 1:
  ...
[[likely]] case 2:
  ...
  break;
case 3:
  ...

Should case 1 be considered likely because it falls through to case 2? From the patch, it looks like your answer is "yes", they should both be likely, but should that hold true even if case 1 does a lot of work and is not actually all that likely? Or is the user expected to write [[unlikely]] case 1: in that situation? We don't seem to have a test case for a situation where fallthrough says something like that:

switch (something) {
[[likely]] case 0:
  ...
  // Note the fallthrough
[[unlikely]] case 1:
  ...
}

In D88314#2322870, @bogser01 wrote:

@aaron.ballman Thank you for picking up this review! Running the check over the entire LLVM causes ~74K warnings across 430 files. As to the false positive rate it's tricky to measure. Based on previous analysis on flang codebase, I would say roughly 50% of the fixes would cause compiler errors when applied.

In D88645#2321980, @jdoerfert wrote:

(Drive By: This is cool)

It looks like you generated a diff from a previous diff instead of trunk -- can you regenerate the diff against trunk so that reviewers can see the full content of the changes?

This LGTM, but I'm not super well-versed with asm statements to begin with. You should wait a bit for other reviewers to weigh in before landing.

Thank you for working on this check! Have you run the check over LLVM to see how much it reports? One of the concerns I have with this is that it's not always appropriate to replace a const std::string& with an llvm::StringRef and so I'm wondering what the "false positive" rate is for the check. For instance, there are std::string member functions that don't exist on StringRef so the fix-it will introduce compile errors, or switching the parameter type will cause additional unnecessary conversions.

In D88833#2319799, @lebedev.ri wrote:

I think the current behavior should instead be configurable, with a way to opt-in into weaker guarantees.

In D88833#2319118, @fiesh wrote:

@aaron.ballman valid point. It would seem natural to diagnose both since the user is voluntarily causing the decay?

LGTM though I may have spotted a potential improvement (it can be handled in a follow-up if you want).

In D69560#2292505, @whisperity wrote:

I'd like to resurrect the discussion about this. Unfortunately, the concept of experimental- group (in D76545) has fallen silent, too. We will present the results of this analysis soon at IEEE SCAM (Source Code Analysis and Manipulation) 2020. While a previous submission about this topic was rejected on the grounds of not being in line with the conference's usual, hyper-formalised nature, with one reviewer especially praising the paper for its nice touch with the empirical world and the fact that neither argument swaps (another checker worked on by a colleague) nor the interactions of this issue with the type system (this checker) was really investigated in the literature for C++ before, we've tailored both the paper and the implementation based on reviewer comments from the scientific world, and the comments here.
The reviews were mostly favourable, excl. comments about the lack of formalisation and the unfortunate lack of modelling for template instantiations. Such a cold cut, however, only introduces false negatives into the result set. Which is fine, as the users will never see those non-existent reports!

Despite my earlier happiness with the patch, it's now a bit unclear to me from the C++ Core Guideline wording what the right behavior here actually is. The bounds profile is trying to ensure you don't access out-of-range elements of a container and the decay part specifically seems to be more about how it impacts pointer arithmetic.

In D88275#2313342, @ymandel wrote:

In D88275#2305989, @aaron.ballman wrote:

In D88275#2303283, @ymandel wrote:

I'm not concerned about the basic idea behind the proposed matcher, I'm only worried we're making AST matching more confusing by having two different ways of inconsistently accomplishing the same-ish thing.

Aaron, I appreciate this concern, but I would argue that this matcher isn't making things any worse. We already have the various ignoringImplicit matchers, and this new one simply parallels those, but for parents. So, it is in some sense "completing" an existing API, which together is an alternative to traverse.

I'm not certain I agree with that reasoning because you can extend it to literally any match that may interact with implicit nodes, which is the whole point to the spelled in source traversal mode. I'm not certain it's a good design for us to continue to add one-off ways to ignore implicit nodes.

I appreciate your point, but I'm personally inclined to allow progress while we figure these larger issues out. That said, I'm in no rush and would like a solution that we're both happy with. How do you propose we proceed, especially given that traverse does not currently support this case? It seems that progress is in part blocked on hearing back from steveire, but it's been over a week since you added him to the review thread. Is there some other way to ping him?

In D87962#2313363, @nomanous wrote:

In D87962#2312617, @aaron.ballman wrote:

LGTM! Do you need someone to commit on your behalf?

Yes I need!

One request though: can you add a release note about the removal with a brief explanation of why it was removed?

LGTM! Do you need someone to commit on your behalf?

LGTM to remove rather than move into readability unless someone has a good argument as to why it belongs there. I don't think this pattern makes code more readable in general as it recommends using a more verbose syntax (pointers, which require dereferencing) over a safer, concise syntax (references, which carry semantic information about nullness).