Page MenuHomePhabricator

[ASTImporter] Import additional flags for functions.
ClosedPublic

Authored by balazske on Aug 9 2019, 3:23 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

balazske created this revision.Aug 9 2019, 3:23 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

LGTM, but I don't exactly see how the first test is related. Could you please explain?

clang/unittests/AST/ASTImporterTest.cpp
5239 ↗(On Diff #214341)

I don't exactly see how this test is related.

balazske marked an inline comment as done.Aug 9 2019, 5:21 AM
balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5239 ↗(On Diff #214341)

I do not remember exactly why this test was added but probably the problem is structural equivalence related: The flags are not imported correctly for the first time, and at the second import structural match fails and a new Decl is created instead of returning the existing one. This test fails if the change is not applied.

Hello Balazs,
The patch looks good in general.

clang/unittests/AST/ASTImporterTest.cpp
5239 ↗(On Diff #214341)

Should we consider isExplicitlyDefaulted() when computing structural equivalence?

5242 ↗(On Diff #214341)

const char * -> StringRef?

5246 ↗(On Diff #214341)

Can we remove the function body or reduce it to 'X x'?

balazske marked 2 inline comments as done.Aug 12 2019, 12:50 AM
balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5239 ↗(On Diff #214341)

We may use isExplicitlyDefaulted and isDeletedAsWritten and isVirtualAsWritten but in another patch.

5246 ↗(On Diff #214341)

The delete x3 is needed to get a destructor for X generated. But the test is about testing the implicit functions and I think it is better to have more of them. This code is there to have these functions generated so I do not like remove of it.

balazske updated this revision to Diff 214585.Aug 12 2019, 1:09 AM
  • Using StringRef, added comment.
balazske marked an inline comment as done.Aug 12 2019, 2:06 AM
balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5239 ↗(On Diff #214341)

It can be good to add check for isExplicitlyDefaulted because it is a separate bit and not checked yet. Probably in another patch that has a test for this too.

a_sidorin accepted this revision.Aug 12 2019, 12:23 PM

Hi Balazs,
The change looks good. I think it can be committed after an unrelated part is removed.

clang/unittests/AST/ASTImporterTest.cpp
5373 ↗(On Diff #214585)

This is a separate functional change and I prefer to move it into a separate patch.

This revision is now accepted and ready to land.Aug 12 2019, 12:23 PM
balazske updated this revision to Diff 214772.Aug 13 2019, 12:14 AM
  • Moved SVEBuiltins test instantiation to non-different position.
balazske marked 2 inline comments as done.Aug 13 2019, 12:17 AM
balazske added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
5373 ↗(On Diff #214585)

This change came in at a rebase to newest master, I wanted to have this instantiation at the end of the list but this showed up as part of the patch so it was moved now back to the start of the list (of all test instantiations).

This revision was automatically updated to reflect the committed changes.
balazske marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 1:03 AM