This is an archive of the discontinued LLVM Phabricator instance.

Changed every use of ASTImporter::Import to Import_New
ClosedPublic

Authored by balazske on Nov 28 2018, 11:57 PM.

Diff Detail

Event Timeline

balazske created this revision.Nov 28 2018, 11:57 PM
balazske updated this revision to Diff 175864.Nov 29 2018, 6:49 AM
  • Changed some missing Import calls.
martong added inline comments.Mar 22 2019, 5:52 AM
lib/AST/ASTImporter.cpp
7767

Actually, this patch is not merely a Import -> Import_New substitution. As this line shows, the error will be propagated correctly. However, in the old version we return with a nullptr DC which may be referenced later. This is the reason, why https://reviews.llvm.org/D59692 breaks in the unittests and thus depends on this patch.

Herald added a project: Restricted Project. · View Herald Transcript

Hi Balazs,

The looks mostly good to me.

lib/AST/ASTImporter.cpp
3440

There is the same deletion in D53757.

8550

Wow, we import types instead of just checking them for structural equivalence. That's OK to leave it in the patch as-is but looks pretty strange. Maybe this even deserves a FIXME.

unittests/AST/ASTImporterTest.cpp
146

Dead code?

balazske marked an inline comment as done.Mar 25 2019, 2:21 AM
balazske added inline comments.
lib/AST/ASTImporter.cpp
8550

The Import call was already here: "ToContext.hasSameType(Import(From), To))"
This is not a big thing, because the type already exists in ImportedTypes, so the Import call will return it.

balazske updated this revision to Diff 192105.Mar 25 2019, 7:45 AM
  • Rebase to current master.
  • Fixed error handling in testImport.
balazske updated this revision to Diff 192110.Mar 25 2019, 8:12 AM

Corrected rebase problems.

balazske marked an inline comment as done.Mar 25 2019, 8:15 AM
a_sidorin accepted this revision.Apr 8 2019, 1:41 AM

LGTM.

This revision is now accepted and ready to land.Apr 8 2019, 1:41 AM
This revision was automatically updated to reflect the committed changes.