... it is highly probable that if some pairs were ever non-equivalent they will stay non-equivalent.

@a_sidorin Alexei,

In D62131#1639168, @balazske wrote:

Example about how to get wrong things into NonEquivalentDecls:
We want to compare class C for structural equivalence in the following codes:

class A {}; class B {int i;}; void x(A *); void y(A *); class C { friend void x(A *); friend void y(A *); };

and

class A {}; class B {int i;}; void x(A *); void y(B *); class C { friend void x(A *); friend void y(B *); };

The result is false for C but the NonEquivalentDecls will contain after the compare a void x(A*)<->void x(A*) pair.

The reason is that during the check we encounter first a A<->B pair (iterating over the friends and friend functions, first y is encountered and a check for A and B follows). The B is recorded as "tentative equivalence" to A. Then we try to check A to A (during check of x) but because there is a "tentative equivalence" with B from A the check returns false (not equivalent). This false result is recorded as a (incorrect) non-equivalence of the two x functions.
I want to replace the DeclsToCheck and TentativeEquivalences with a set of Decl pairs (like the NonEquivalentDecls) so it will be possible to record the same from Decl with multiple to values. (Is there a reason for why there is a NonEquivalentDecls cache but not a EquivalentDecls or simply a cache for result values?)

There is a third test which could be useful to test whether there is no faulty cache entries there:

I like this patch because it eliminates the need for checking the redeclaration chains.

@shafik Ping

The link for the diff went off, sorry about that.
Here is the new link which is going to work:
https://github.com/martong/clang/compare/NonEqDecls_cache_in_an_upper_level_0...martong:NonEqDecls_cache_in_an_upper_level?expand=1

It looks like that the original code is correct in the decision of structural equivalence of the original pair. If we have an (A,B) and (A,C) to compare, B and C are in different decl chain, then (A,B) or (A,C) will be non-equivalent (unless B and C are equivalent, but what to do in this case?). The problem was that the code assumed that in this case always A and B (or A and C) are non-equivalent. If NonEquivalentDecls is not filled in this case (or not used at all) the problem does not exist.

• Address review comments

Thanks for the review!

In D66348#1636564, @shafik wrote:

I am not enthusiastic about this solution but I need to think about it some more.

We can see that p0624r2 added copy assignable lambdas:

bool f1() {
    auto x = []{} = {}; auto x2 = x;

    return x == x2;
}

bool f2() {
    auto x = []{} = {};
    auto xb = []{} = {};

    return x == xb;
}

see godbolt

So I don't think this is a long-term solution, although I don't know what clang is doing to make this work yet.

• Add tests for default constructible and assignable stateless lambdas

In D64078#1572675, @a_sidorin wrote:

Hi Gabor,
it is a nice design question if source locations can participate in structural match or not. The comparison tells us that the same code written in different files is not structurally equivalent and I cannot agree with it. They can be not the same, but their structure is the same. The question is: why do we get to this comparison? Do we find a non-equivalent decl by lookup? I guess there can be another way to resolve this issue, and I'll be happy to help if you share what is the problem behind the patch.

• Name -> SearchName
• Add tests for conflicting declarations

In D65573#1625881, @a_sidorin wrote:

That's incredible. Thank you!

In D59692#1632138, @shafik wrote:

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

LGTM!
Thank you.

LGTM, but I don't exactly see how the first test is related. Could you please explain?

Would it be an alternative solution if we imported the function parameters after we created the FunctionDecl? I guess it would be. Maybe that would result in a smaller change, what do you think?

Still looks good to me, other than some style nits.

• Add description for -ast-merge

• Address comments

Thanks for the review!

LGTM, though I have some comments.

LGTM! Thanks! Please do the commit.

LGTM! Please proceed and commit! :)

In D65445#1608277, @balazske wrote:

Probably this is a problem case too but only if the f or i has an initializer expression and really no USR is generated for f or i. But what I have found is that there is a VarDecl for a "invisible" variable whose type is the anonymous union and it has a (default) initializer too. If the search reaches DC of the f it will try to get the USR for this variable.

In D65445#1607803, @balazske wrote:

It looks like that the problem can happen when the anonymous union is in any DeclContext and for CTU import the import of a variable is requested and that variable is in a related DeclContext (it can be at upper or lower level). (See code of findDefInDeclContext: If used for variables, it goes through every DeclContext and computes USR for specific VarDecls until the one is found. If we have luck it may be found before the union. An anonymous union can not be imported because we get no USR for it so it is OK to ignore these.) So the test can be written in many other forms but this case (the code of f) was encountered in protobuf code.

Thank you for this patch Balazs! Could you please elaborate in which cases we cannot generate the USR? Does it happen only if we have an anonymous union inside a function? Are there no other cases?

@a_sidorin This is a polite Ping.

Jenkins is green: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31743/

I have a feeling that LoadPass and LoagGuard could be (should be) merged together, other than that we are getting close.

• Address Alexei's comments

• Rebase to master

In D64638#1593382, @ilya-biryukov wrote:

StaticAnalyzer/Core does not depend on clangFrontend now, you can see this by looking at lib/StaticAnalyzer/Core/CMakeLists.txt:

add_clang_library(clangStaticAnalyzerCore
...
  LINK_LIBS
  clangAST
  clangASTMatchers
  clangAnalysis
  clangBasic
  clangCrossTU
  clangLex
  clangRewrite
  )

Not a StaticAnalyzer expert, so I don't know whether it's acceptable to add this dependency to clangStaticAnalyzerCore, you'll have to find someone who owns the code to know whether this dependency is justified.
(My wild guess from looking at the names of the libraries would be that this dependency is not ok and the code should go into clangStaticAnalyzerFrontend instead. But again, not an expert here, just a guess).

But please add a dependency into LINK_LIBS inside CMakeLists.txt if you start depending on clangFrontend.
Most of these violations are found if you build in a cmake -DBUILD_SHARED_LIBS=On configuration.

@a_sidorin This is a polite Ping.

@a_sidorin, @shafik This is a polite Ping.

LGTM! Thanks!

ASTImporter.cpp and ASTStructuralEquivalence.cpp looks good to me!

• Further simplify by removing the last for loop

• Rebase to master
• Some refactor is done mostly because since D63603 ([ASTImporter] Propagate

error from ImportDeclContext) we may not imported successfully all decls of a
DC.

• Made the code simpler and shorter by adding a local vector to hold the decls

of the "from" DC in the original order.

In D64241#1584972, @a_sidorin wrote:

Hi Gabor,
This looks fine, but could you please add a test showing how decl shadowing is handled? I.e. if we have Arg in one TU and both Arg and N::Arg in another TU.

• Add test case ClassTemplSpecWithInequivalentShadowedTemplArg

• Add notes about macro expansion crash with CTU

LGTM.

Jenkins looks okay: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31160/ .The build is red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py

• Address dkrupp's comments

@shafik I've been looking for any lldb regression in our Mac machine, could not find any. Now I am looking at http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/ .

Jenkins looks okay: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/31157/ .The build is red but the the previous run has the very same failing test case:
expression_command/weak_symbols/TestWeakSymbols.py

Thank you guys for the review!

• Applied clang-format on lldb parts (this changed two lines)
• Added a comment for predicate
• Merged the test into TestCModules.py