Page MenuHomePhabricator

[ASTImporter] Fix missing implict CXXRecordDecl in ClassTemplateSpecializationDecl
ClosedPublic

Authored by martong on May 18 2018, 3:41 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

martong created this revision.May 18 2018, 3:41 AM
a.sidorin added inline comments.May 18 2018, 11:35 AM
lib/AST/ASTImporter.cpp
1962 ↗(On Diff #147466)

We are changing import if RecordDecl. Is it possible to add a test that doesn't require templates?
I tried and found that the implicit CXXRecordDecl of ClassTemplateSpecializationDecl is its redeclaration. That's not true for normal CXXRecordDecls, as I see, so this deserves a comment.

martong updated this revision to Diff 147994.May 22 2018, 6:35 AM
martong marked an inline comment as done.
  • Add new test case for simple CXXRecordDecl
  • Add comment about ClassTemplateSpecializationDecl implicit CXXRecordDecl
lib/AST/ASTImporter.cpp
1962 ↗(On Diff #147466)

Yes, I've added a new test which exercises only a RecordDecl.
Also, added a comment about the specialization re-declaration.

a.sidorin accepted this revision.May 22 2018, 9:31 AM

LGTM with a nit.

lib/AST/ASTImporter.cpp
1962 ↗(On Diff #147994)

Multiline comments are pretty uncommon in LLVM. Could you please replace it with // before commit? I.e.

if (Definition && Definition != D &&
    //
    //
    !D->isImplicit())
This revision is now accepted and ready to land.May 22 2018, 9:31 AM
martong updated this revision to Diff 148209.May 23 2018, 7:26 AM

Remove multiline comment and reorder parts of the condition

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.