Page MenuHomePhabricator

[ASTImporter] FriendDecl importing improvements

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



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!


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


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?


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


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


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


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

This will assert unless the IdentifierNamespace is properly imported. See

Note that changes from 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.


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.