I am in contact with the guy that actually discovered this breakage. It seems to be a weird and hard to reproduce error, but happens on x86 as well. So it is a compile/header something combination that results in the error. I am pretty sure that will take a while until we figured it out :/

Unfortunatly this check does not compile on some ARM platforms. It seems that a matcher exceeds the recursion limit in template instantations.

In D53187#1264415, @baloghadamsoftware wrote:

I think further optimization steps should be done is separate patches. However, this is the biggest step.

Committed with https://reviews.llvm.org/rCTE344374

The patch does not apply clean, could you please take a look at it?

• make the logic with variadic clear

• remove unused enum in header file, no idea what i intended to do with it :D

Updated

• add tests and adjust doc
• one more test case

In D52892#1263324, @aaron.ballman wrote:

I like the thrust of this check, but we already have the readability-magic-numbers check and I think that this functionality would fit naturally there. That check finds numerical constants and recommends turning them into named constants, but for special values it seems reasonable to recommend using numeric_limits instead. What do others think of that, from an organizational perspective?

• fix headline in doc

Thousands? After the query optimization the max was 173, and that only for a single function. The next number was 64.

Unfortunately, I won't be able to review the code in the upcoming few weeks as I caught a cold and I'm trying to get better before the LLVM Meeting, so if there is anyone else interested in reviewing proposed changes please feel free to jump in.

In D53187#1263294, @baloghadamsoftware wrote:

I think that with this optimization it is not so expensive anymore. I do not think it was an endless loop in the bugreport but it was insufferable execution time. Maybe we could speed it up a little more by changing it totally to a width-first CFG visitor. Then we could apply your solution as well (not removing visited function from the call stack) so the algorithm would visit every called function (for every function that should not throw) only once along the shortest path. This would not introduce new false positives neither would it lose true positives.

I see, probably not worth it. Its one-time effort anyway right?
LGTM

Did you verify it actually works?
Otherwise LGTM and because its a bug-fix you can commit and other concerns can be done post-commit.

This looks reasonable to me but could you please explain a bit what the issue was in the bugreport? So a very deep hierarchy causing the problem makes sense, but why was "ignoring first" the difference maker?
Would it make sense to add a warning in the documentation that this check can be very expensive? And are there measures to speed the process up somehow?

LG in principle, just the SmallVec thing could be done if you agree. I don't insist on it, but it looks like a performance benefit to me.

In D46027#1259989, @ZaMaZaN4iK wrote:

What is the status of the PR?

welcome to the LLVM community and thank you very much for working on that check!
If you have any questions or other issues don't hesitate to ask ;)

The change looks good in principle. I think it would make sense to migrate one test already, to use the new capability and check if everything works as expected. The current tests still run fine?

• address review comments, simplifying code

@kbobyrev is it ok for you if I stick with the Optional<> style in the check?

I think you can commit, there was enough opportunity to respond and we pinged directly as well.

In D52527#1257277, @owenpan wrote:

I'd greatly appreciate it if someone could review this before I commit it next week.

C.131 seems to imply a minimal amount of trivial getters/setters before
diagnosing.

In D52771#1256432, @aaron.ballman wrote:

I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you have thoughts on how to avoid that? The C++ Core Guidelines seem silent on the matter -- this might be worth raising with the authors.

Commited in https://reviews.llvm.org/rL343848.
Thank you for the patch!

@aaron.ballman I have a question for an expert: How would bitfields relate to this check? Can there be a similar pattern for them and do they need to be handled here?

This patch does not fix the underlying issue. I do have a candidate that might cause it, but I am not sure if it is functionally preserving (tests seems to work though!). I think it is best if @baloghadamsoftware takes a look at it as well!

thanks for working on this! I have one question: Why are variables _not_ considered in the check but only constants? IMHO it would make sense to transform these as well.

from my side is nothing outstanding

Committed on your behalf

In D52880#1255250, @alexfh wrote:

In D52880#1255249, @alexfh wrote:

Have you figured out why exactly does the check hang? Disabling it for -fno-exceptions may just hide a logical problem in the check.

(disabling it for these cases seems to be the right thing to do anyways, but it might be nice to use the opportunity to investigate the possible issue deeper)

In D52880#1255031, @lebedev.ri wrote:

Needs a test.

This should definitly be backported, as exception-escape is in 7.0 and it hangs up on no-except builds

In D52782#1254945, @mboehme wrote:

In D52782#1254933, @JonasToth wrote:

Thanks for clarification :)

Thanks! Do you agree this is ready to land now?

Thanks for clarification :)