Page MenuHomePhabricator

[ASTImporter] Corrected import of repeated friend declarations.
Needs ReviewPublic

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
3635

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

3636

const FriendDecl *FD?

3639

const?

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

?

3715

Why not strictly equal?

clang/unittests/AST/ASTImporterTest.cpp
4009

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

4034

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

clang/unittests/AST/StructuralEquivalenceTest.cpp
801

We need to place spaces after foo.

811

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
3640

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

3715

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
3638

uint32_t

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

3640

uint32_t

3644

uint32_t

3660

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
4009

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

4034

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
3638

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?