Changes based on comments.

Format changes added based on comments.

Yepp, good point, Aleksei, rewritten the tests using explicit instantiation.

I have commited this patch (r330847) but unfortunately "refactored" the differential revision link from the commit message out. Added it as a dependent commit.

Yepp, pretty straightforward check for something we were not aware previously (but unfortunately encountered it).

Changes made based on comments.
The CFG recreating problem is handled the following (only for this check):
Always store the last visited function and its CFG* (in form of the Sequence*) and check if we are visiting it again. If so, then the check reuses the previous one, if not, then replaces them. As far as I know the AST traverse done by the tidy fits this model (at least for this check, since it not uses narrowing matchers to other functions).
Sure, it would be better to find a general solution to this problem, and make the CFG reusable by every check which needs it, but I would left it for a follow-up (and a change like this probably would worth an own patch/review anyway).

Rewritten the tests using the newly added TEST_P method.
This patch failed earlier when -fdelayed-template-parsing flag was enabled, however, the actual import process was OK but the original AST havent included the checked nodes.
TEST_P made possible to check whether the original code contains the node we would like to import (and we test only in this case, since otherwise it does not make sense).

Hello Aleksei,

Addressed comments and readded the lost fixes.

Rebase + set the IDNS in a more general way.

Moved to bugprone category,
skipping memberExpr cases for now in order to avoid false positives,
other small changes based on review comments.

Yepp, thanks for the patch! One small typo below.

Added the assignment operator matcher patch as a dependency and rebased on top of that.

In D38921#1037180, @alexfh wrote:

Do you want to submit the matcher part separately?

Nice catch, it looks good to me! Thank you!
One small nit for future debugging people: Could you insert a comment line in the test case where you explain what is this all about? E.g what you just have written in the description: "invalidateRegions() will construct the SymbolConjured with null Stmt" or something like this.

In D16403#1011218, @NoQ wrote:

Yeah, i mean, like, if we change the scope markers to also appear even when no variables are present in the scope, then it would be possible to replace loop markers with some of the scope markers, right?

ping

ping

In D16403#992452, @NoQ wrote:

Thank you, this explanation looks very reasonable.

All right, so right after the termination of the loop we have

[B1]
1: ForStmt (LoopExit)
2: [B4.5].~A() (Implicit destructor)
3: [B5.3].~A() (Implicit destructor)
4: CFGScopeEnd(a)
5: CFGScopeEnd(b)

... where [B4.5] is A b = a; and [B5.3] is A a;. Am i understanding correctly that while destroying a you can still use the storage of b safely? Or should a go out of scope before b gets destroyed? Also, is the order of scope ends actually correct here - shouldn't b go out of scope earlier? Given that they are in very different lifetime scopes (a is one for the whole loop, b is per-loop-iteration). I guess the order would matter for the analyzer.

Would it make sense to use the last element of the block edge's source for the diagnostic location when the destination block is empty?

First, sorry for this delayed update, however, I was working on this and running this on real projects. I wanted to make sure that this update will be complete enough that this patch would not cause any harm (yes, these features are hidden behind a flag but anyway) and not crashes on edge cases I haven't thought of. The core used the fact that LocationContext only contains StackFramce and BlockInvocation and I aimed to eliminate all these code snippets (more like rewrite).

I essentially have one question at a glance - for loop counter variables, don't we want LoopEntrance be before the initialization?

Added comments and removed indirect goto support from this patch.

Comment added to LoopEntrance CFGElement.

In D41444#961110, @a.sidorin wrote:

Test both with and without '-fdelayed-template-parsing' in C++ mode.

Hi Alexey!

Nope, that was me. If this (and the dependencies) are looking good, I can commit them as well (added the dependencies ^^).

In D40937#947760, @JVApen wrote:

How does this check deal with atomic members?
...

Updates based on comments.

Hi Eugen!
Good question, probably should have detailed it in the description.
This matcher based solution would not gain much benefit from the symbolic execution provided information. (I mean, it would mean a much different type of check on the states.)
The main problems that the analyzer does not completely unroll the loops only the first steps and we always have information about the simulated path. However, detecting that some variables will surely not be modified requires a top level overview on the loop and the AST provides these informations. The one thing (that I can say right now) that can come handy is that we would able to detect more precisely the happened-before relation on the escape and the loop statements. Since the CFG can provide us fair enough information on this one, I do not think that this is enough reason to put this checker to the analyzer.

Updated the wording in the docs.

Added Alexander and Aaron as reviewers for the matcher parts.

Yepp, I am not exactly sure how I managed to do it, but I've missed this comment last time. Fixed it.

Thank you for the review!

Updating the usage of ImportTemplateArgumentListInfo.

Updating the usage of ImportTemplateArgumentListInfo.

Updated based on review comments.

Hello Aleksei,

Updates based on review comments.

Left some unformatted lines in the previous version. Fixed.

Updates based on comments.

Testfiles added and HTML updated.

isAssignmentOp matcher moved to ASTMatchers.h
(Manuel added to reviewers as the code owner of ASTMatchers)

• Updated to use a custom AST matcher for assignment operator check. (More structured and efficient.)
• Tests added as well.

Just removed some accidentally left changes from the patch.

LGTM as well, but same note applies.

Checking for importing base updated to (!Imported && From) style.

Fixed the commented bug (check if ThisRegion is null).

Test file (running line) update.