This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] FriendDecl importing improvements
AbandonedPublic

Authored by szepet on May 6 2017, 1:50 PM.

Details

Summary

Set FriendObjectKind in case of decls.
RD->pushFriendDecl(FrD); statement removed from L2240 since it is done in function Create(..).
IsStructurallyEquivalent function(s) of FriendDecl and dependent decls (RecordDecl, FunctionDecl) added.
Test cases added,

Diff Detail

Event Timeline

szepet created this revision.May 6 2017, 1:50 PM
spyffe requested changes to this revision.Jun 14 2017, 10:56 AM

Sorry for the late review, I saw your ping but the last week was busy. Anyway, thanks for the tests!

lib/AST/ASTImporter.cpp
2660

Since getFriendObjectKind() returns an enum, in my opinion it's better style to check explicitly whether it != FOK_None.

2663

From a close reading of setObjectOfFriendDecl(), it looks to me like we might not set the IDNS_Tag or IDNS_Ordinary bits in IdentifierNamespace since we're passing PerformFriendInjection = false and since getPreviousDecl() might return nullptr in ToContext.

This means that getFriendObjectKind() might return FOK_Undeclared whereas it returned FOK_Declared for the original. Does this matter?

lib/AST/ASTStructuralEquivalence.cpp
966

Would it be appropriate to emit a diagnostic here as well?

1228

What if D1->getFriendDecl() != nullptr but D2->getFriendType() != nullptr or vice versa?
I wouldn't want to return true in that case…

1238

For attributes on the function (e.g., __attribute__ ((always_inline))) are these relevant to structural equvalence? Or does really only the type matter?

1477

What about if FrD1->getFriendDecl() && FrD2->getFriendDecl()?

This revision now requires changes to proceed.Jun 14 2017, 10:56 AM
szepet updated this revision to Diff 117113.Sep 29 2017, 3:03 AM
szepet edited edge metadata.
szepet marked 6 inline comments as done.

Updates based on comments.

dkrupp added a reviewer: bruno.Nov 3 2017, 8:51 AM

Could you add some context?

Generally LGTM!

One high level comment, I think we need to modify the importing of CXXRecordDecls to properly support friend declarations. When we are building the redeclarable chain of CXXRecordDecl declarations I suspect we also need to consider names in the IDNS_TagFriend namespace.

martong added inline comments.Feb 19 2018, 1:02 PM
lib/AST/ASTImporter.cpp
2260

This will assert unless the IdentifierNamespace is properly imported. See https://github.com/Ericsson/clang/pull/266/files#diff-7f5f26dbdb3322c6308b1018ff0dbe5aR6735

Note that changes from https://reviews.llvm.org/D44100 might affect this.

szepet updated this revision to Diff 141134.Apr 5 2018, 5:39 AM

Rebase + set the IDNS in a more general way.

martong accepted this revision.Apr 25 2018, 2:02 AM
martong added reviewers: martong, xazax.hun.

LGTM!

Looks good to me as well. Thank you!

a.sidorin accepted this revision.Apr 25 2018, 2:14 AM
szepet abandoned this revision.Apr 26 2018, 4:24 AM

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