This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Handle redecl chain of FunctionTemplateDecls
ClosedPublic

Authored by martong on Feb 21 2019, 2:08 AM.

Details

Summary

Redecl chains of function templates are not handled well currently. We
want to handle them similarly to functions, i.e. try to keep the
structure of the original AST as much as possible. The aim is to not
squash a prototype with a definition, rather we create both and put them
in a redecl chain.

Diff Detail

Event Timeline

martong created this revision.Feb 21 2019, 2:08 AM

Hi Gabor,
The patch looks OK overall but I have some comments inline.

lib/AST/ASTImporter.cpp
4966

We should point that this function is for TemplateDecls only somehow. But we can't just pass TemplateDecl as the parameter due to loss of the actual return type. Maybewe should rename this function into "getTemplateDefinition()"?

5563

We don't usually put such comments after control flow statements. If they are really needed, it is a good sign that a function must be split, and it's better to leave a FIXME for this (or do the split).

martong updated this revision to Diff 188379.Feb 26 2019, 8:19 AM
martong marked 4 inline comments as done.
  • getDefinition -> getTemplateDefinition
  • Remove comments for braces, added FIXME
martong added inline comments.Feb 26 2019, 8:20 AM
lib/AST/ASTImporter.cpp
4966

Ok, I changed that to getTemplateDefinition.

5563

Ok, I removed these comments and added a FIXME.

a_sidorin accepted this revision.Feb 26 2019, 1:59 PM

Thanks for the fixes!

This revision is now accepted and ready to land.Feb 26 2019, 1:59 PM

@shafik, Could you please take a look?

I have run the LLDB tests on our macOS and I could not discover any regression.

shafik accepted this revision.Mar 5 2019, 11:06 AM

LGTM outside of the question I had.

lib/AST/ASTImporter.cpp
4967

Can we guarantee that D->getTemplatedDecl() will always return a valid pointer?

4967

What other types besides CXXRecordDecl do we expect here?

5543

Would it make sense to do the split into a separate function in the PR?

5593

Can we guarantee that FoundByLookup->getTemplatedDecl() will always return a valid pointer?

martong updated this revision to Diff 189665.Mar 7 2019, 1:50 AM
martong marked 6 inline comments as done.
  • Add asserts
martong added inline comments.Mar 7 2019, 1:50 AM
lib/AST/ASTImporter.cpp
4967

Can we guarantee that D->getTemplatedDecl() will always return a valid pointer?

Actually, we always call this function on a ClassTemplateDecl or on a FunctionTemplateDecl which should return with a valid pointer. Still it is a good idea to put an assert at the beginning of the function, so I just did that.

assert(D->getTemplatedDecl() && "Should be called on templates only");

What other types besides CXXRecordDecl do we expect here?

FunctionDecl can be there too when we call this on a FunctiomTemplateDecl param.

5543

I'd rather do that in another patch where we could address the similar issues in other visit functions too. Actually, most of the visit functions start with a lookup and then continue with a loop over the lookup results; this part could be split into a separate function everywhere.

5593

This is a good catch, thanks.
The FoundByLookup variable's type is FunctionTemplateDecl. Either the found Decl is created by the parser then it must have set up a templated decl, or it is created by the ASTImporter and we should have a templated decl set.
In the second case, however, we may not be 100% sure that the templated is really set.
So I have added an assert here too.

This revision was automatically updated to reflect the committed changes.