Page MenuHomePhabricator

[ASTImporter] Corrected import of repeated friend declarations.
ClosedPublic

Authored by balazske on Mar 6 2020, 6:20 AM.

Details

Summary

Import declarations in correct order if a class contains
multiple redundant friend (type or decl) declarations.
If the order is incorrect this could cause false structural
equivalences and wrong declaration chains after import.

Diff Detail

Event Timeline

balazske created this revision.Mar 6 2020, 6:20 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Hello Balazs,
This look almost good to me except some comments inline.

clang/lib/AST/ASTImporter.cpp
3649

It's better to turn the tuple into a named struct (FriendCountAndPosition?) to make the code more readable.

3650

const FriendDecl *FD?

3653

const?

3654
if (const TypeSourceInfo *FriendType = FD->getFriendType()) {
    QualType TypeOfFriend = FriendType->getType().getCanonicalType();
...

?

3729

Why not strictly equal?

clang/unittests/AST/ASTImporterTest.cpp
3978

clang-tidy wants this to be const auto* Code.

4003

clang-tidy wants this to be const auto* Code.

clang/unittests/AST/StructuralEquivalenceTest.cpp
763

We need to place spaces after foo.

773

Could you please add a positive test with two struct foo{ friend class X; friend class Y; };", structures as well?

balazske updated this revision to Diff 249100.Mar 9 2020, 7:43 AM
balazske marked 2 inline comments as done.

Fixes according to comments.

balazske marked 9 inline comments as done.Mar 9 2020, 7:47 AM
balazske added inline comments.
clang/lib/AST/ASTImporter.cpp
3654

In the current way the branches (after if and else) look relatively similar, I like that better.

3729

D id the Decl to be imported, ImportedEquivalentFriends contains the similar friends already imported. This may be less than the total (0 at first time) if not all friends are imported yet (should be a temporary state). The next line checks for equality.

balazske marked 2 inline comments as done.Mar 9 2020, 7:55 AM

LGTM WDYT @teemperor

clang/lib/AST/ASTImporter.cpp
3652

uint32_t

Is there a reason to not prefer fixed width integer types?

3654

uint32_t

3658

uint32_t

3674

Can we refactor to not repeat code? They look identical. Duplicate code is a source for errors when once piece of code is fixed both the other is not etc

clang/unittests/AST/ASTImporterTest.cpp
3978

Does auto really buy us anything here? Why not use use const char*?

4003

const char*?

balazske updated this revision to Diff 249317.Mar 10 2020, 4:55 AM
balazske marked an inline comment as done.

Removed code repetition.
Using const char * instead of auto.

balazske marked 3 inline comments as done.Mar 10 2020, 4:57 AM
balazske added inline comments.
clang/lib/AST/ASTImporter.cpp
3652

I do not know about any reason, but in ASTImporter.cpp uint32_t does not appear but unsigned and unsigned int yes. Why can be fixed width type better that the default-sized int?

shafik accepted this revision.Jul 2 2020, 1:39 PM

LGTM

This revision is now accepted and ready to land.Jul 2 2020, 1:39 PM
This revision was automatically updated to reflect the committed changes.