In D68634#1701192, @martong wrote:

In D68634#1700591, @a_sidorin wrote:

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

I was thinking about that too, but it turned out to be a way more intrusive change.

We have to work with the most recent existing decl (PrevDecl) of the FunctionDecl whose attribute we currently import.
We can get the PrevDecl only with the help of the lookup. We can't get it from the ToD param of InitializeImportedDecl because by the time when we call InitializeImportedDecl the redecl chain is not connected. So, we should pass PrevDecl then into GetImportedOrCreateDecl. It would require to change all call expressions of GetImportedOrCreateDecl (58 occurences). Besides, we would use the PrevDecl only in case of inheritable attributes, only they require this special care. Note that there are a bunch of non-inheritable attributes which are already handled correctly in InitializeImportedDecl.

In D68634#1700591, @a_sidorin wrote:

Hello Balasz,
In my opinion, importing and setting the attributes should be handled by the stuff used in InitializeImportedDecl(). Can we extend it or reuse the code? It will allow us not to miss the required actions while importing a new function too.

LGTM!

With [modern-type-lookup], we completely evade the use of ASTImporterDelegate? That would be a wonderful thing to use only the the ExternalASTMerger on a long term...

Raphael, thanks for working on this. Overall, the changes look good to me, but please see my comment for the constructor.

2nd attempt to fix the windows build errors: 372646

Trying to fix in svn commit 372633.

BTW, I tried to access the bug reports rdar://55502701 and rdar://55129537, but could not.
I tried at openradar, but there the search for the ID was not successful.
I wonder if there is a publicly accessible (for non Apple employees) URL for these bugs?

Thanks for the thorough explanation about the different ASTContexts in LLDB.
The hack is indeed hideous, but seems good to me... for now until the real solution is born.

Looks good to me, thanks!

• PrevF->ImportedF, remove braces

I have reorganized the tests to group the ones which test ODR violation strategy independent behaviour.

• Reorganize ODRStrategy independent behaviour tests.

• Initialize the variable template in test
• Clean up tests for fun templates and for specializations

In D66951#1660886, @balazske wrote:

OK
Probably the ClassTemplateSpec can not be handled in liberal way because the AST data structure for template specializations do not allow multiple instances with same argument values?

• Fix wrong file comment

Ping

@a_sidorin Ping

• Fix copy error

LGTM, other than a few comments.

LGTM!

In D63640#1648700, @Tyker wrote:

Sorry for the long wait.

Changes:

• Rebased on current master
• Duplicated test file so that it runs for both importing

Looks good, I just have a comment about the matcher.

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

• Apply clang-format

• Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl
• Remove test suite ConflictingDeclsWithConservativeStrategy

• Fix typo: getTemplatedDec -> getTemplatedDecl

In D66538#1642999, @balazske wrote:

In D66538#1642883, @martong wrote:

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

+TEST_F(StructuralEquivalenceCacheTest, DISABLED_NonEq) {
...
+}

This is somewhat the same check that is done in the current tests when the NonEquivalentDecls is checked to not contain any pairs of equivalent Decls. (Specially this line:

EXPECT_FALSE(isInNonEqCache(
      findDeclPair<FunctionDecl>(TU, functionDecl(hasName("x")))));

)

@shafik I have updated the patch with ODR handling strategies as per our discusson.

• Use resulting Name from HandleNameConflict if set
• Add ODRHandling strategies
• Refactor tests, to avoid some code repetition

... 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