This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Support LambdaExprs and improve template support
ClosedPublic

Authored by a.sidorin on Jan 19 2018, 8:17 AM.

Details

Summary

Also, a number of style and bug fixes was done:

  • ASTImporterTest: added sanity check for source node
  • ExternalASTMerger: better lookup for template specializations
  • ASTImporter: don't add templated declarations into DeclContext
  • ASTImporter: introduce a helper, ImportTemplateArgumentListInfo getting SourceLocations
  • ASTImporter: proper set ParmVarDecls for imported FunctionProtoTypeLoc

Diff Detail

Repository
rC Clang

Event Timeline

a.sidorin created this revision.Jan 19 2018, 8:17 AM

High level note: clang::TypeAliasDecl has the same issue as CXXRecordDecl, so templated versions should not be added to the DeclContext. Could you add that just for the sake of completeness?

I do not see a test for the following changes:

  • ASTImporter: don't add templated declarations into DeclContext
  • ASTImporter: proper set ParmVarDecls for imported FunctionProtoTypeLoc
lib/AST/ASTImporter.cpp
2085

Use auto to avoid repeating type.

2100

Use auto to avoid repeating type.

2160

Is the switch above meant to be exhaustive? If so, adding LLVM_UNREACHABLE here might be beneficial.

6140

Use auto to avoid repeting type.

lib/AST/ASTStructuralEquivalence.cpp
1289

The name if this function might be misleading. Only looking at the name I would assume that it is also checking for the templated decl itself not just the identifier and the template parameters. I think it would be nice to either change the functionality or the name.

I do not see a test for the following changes:

  • ASTImporter: don't add templated declarations into DeclContext

It's in ASTImporterTest. It checks that the templated decl cannot be found in the enclosing TU.

  • ASTImporter: proper set ParmVarDecls for imported FunctionProtoTypeLoc

test/ASTMerge/function-cpp. If you remove the change, Sema will segfault.

I do not see a test for the following changes:

  • ASTImporter: don't add templated declarations into DeclContext

It's in ASTImporterTest. It checks that the templated decl cannot be found in the enclosing TU.

Oh indeed, it looks likeI missed that, thanks :)

  • ASTImporter: proper set ParmVarDecls for imported FunctionProtoTypeLoc

test/ASTMerge/function-cpp. If you remove the change, Sema will segfault.

Neat!

a.sidorin updated this revision to Diff 131279.Jan 24 2018, 8:39 AM

Addressed review comments

High level note: clang::TypeAliasDecl has the same issue as CXXRecordDecl, so templated versions should not be added to the DeclContext. Could you add that just for the sake of completeness?

I'd rather create a separate patch - this one is already large enough. Is it OK?

I'd rather create a separate patch - this one is already large enough. Is it OK?

Yeah, that is fine by me.

xazax.hun accepted this revision.Jan 25 2018, 2:28 AM

Overall looks good! Thanks for working on this!

lib/AST/ExternalASTMerger.cpp
403

Usually, we expect that Import calls might fail and return nullptrs. If this is special for some reason I think it worth a comment.

This revision is now accepted and ready to land.Jan 25 2018, 2:28 AM

Thank you, Gabor. I'll change cast<> to cast_or_null<> to enable the assertion below.

This revision was automatically updated to reflect the committed changes.