This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Improve handling of incomplete types
ClosedPublic

Authored by spyffe on May 8 2017, 2:56 PM.

Details

Summary

ASTImporter has some bugs when it's importing types that themselves come from an ExternalASTSource. This is exposed particularly in the behavior when comparing complete TagDecls with forward declarations. This patch does several things:

  • Adds a test case making sure that conflicting forward-declarations are resolved correctly;
  • Extends the clang-import-test harness to test two-level importing, so that we make sure we complete types when necessary; and
  • Fixes a few bugs I found this way. Failure to complete types was one; however, I also discovered that complete RecordDecls aren't properly added to the redecls chain for existing forward declarations.

Diff Detail

Event Timeline

spyffe created this revision.May 8 2017, 2:56 PM
spyffe updated this revision to Diff 98218.May 8 2017, 2:59 PM

Eliminated a missing newline at end of file error

a.sidorin accepted this revision.May 10 2017, 5:42 AM

Looks good!

lib/AST/ASTImporter.cpp
1757

By the way, should we do the same for some other kinds of Decls (like FunctionDecl)? If so, could you write a NOTE comment?

This revision is now accepted and ready to land.May 10 2017, 5:42 AM
spyffe updated this revision to Diff 98497.May 10 2017, 11:21 AM

• Added a FIXME per Aleksei Sidorin's suggestion.

spyffe marked an inline comment as done.May 10 2017, 11:22 AM
spyffe added inline comments.
lib/AST/ASTImporter.cpp
1757

I added this as a FIXME, in the style of the other notes about incomplete implementation in this file.

bruno accepted this revision.May 10 2017, 3:14 PM

Hi Sean,

LGTM! One comment below.

tools/clang-import-test/clang-import-test.cpp
263

No need for the curly braces here

spyffe marked 2 inline comments as done.May 12 2017, 3:15 PM
spyffe added inline comments.
tools/clang-import-test/clang-import-test.cpp
263

Okay.

dkrupp added a subscriber: dkrupp.Jul 17 2017, 4:30 AM

Is this blocked on something?

szepet closed this revision.Oct 5 2017, 5:50 PM
szepet added a subscriber: szepet.

This patch was commited in r302975. (https://reviews.llvm.org/rL302975)