This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Fix lexical DC for templated decls; support VarTemplatePartialSpecDecl
ClosedPublic

Authored by a.sidorin on Feb 7 2018, 3:14 AM.

Diff Detail

Repository
rC Clang

Event Timeline

a.sidorin created this revision.Feb 7 2018, 3:14 AM
xazax.hun accepted this revision.Feb 9 2018, 2:49 AM

Looks good to me. Only found a few nits.

lib/AST/ASTImporter.cpp
4296–4297

auto * to not repeat type.

4399

The formatting might be a bit off here.

This revision is now accepted and ready to land.Feb 9 2018, 2:49 AM
a.sidorin marked 2 inline comments as done.Feb 9 2018, 8:23 AM
a.sidorin added inline comments.
lib/AST/ASTImporter.cpp
4296–4297

I usually prefer to keep the type if it doesn't give a large space win because it hurts readability a bit. From VarDecl *, we can instantly find the type; for auto, we have to look forward. (Yes, VarTemplatePartialSpecializationDecl has to be replaced immediately :) ).
Other issue is that QtCreator I use for development still doesn't have an autocompletion for auto types. However, LLVM says: "do use auto with initializers like cast<Foo>(...)", so I'll change this.

a.sidorin updated this revision to Diff 133626.Feb 9 2018, 8:29 AM
a.sidorin marked an inline comment as done.

Fix style issues found on review.

martong added inline comments.Feb 10 2018, 9:12 AM
lib/AST/ASTImporter.cpp
2858

In case of class templates, the explicit instantiation is the member of the DeclContext. It does not belong to the DeclContext only in case of implicit instantiations.
I suppose the same is true for template variables as well.
In our code base we fixed it by checking on the TSK_ kind, see https://github.com/Ericsson/clang/pull/270/files.

@xazax.hun perhaps we should open source some of our fixes as well?

4455

This is related to my other comment, perhaps we should not add D2 to the DeclContext unconditionnally. I think only implicit instantiations should be added. See
https://github.com/Ericsson/clang/pull/270/files

Just ignore my previous comments, the issue with explicit instantiations could be fixed in a separate independent patch. All is good.

a.sidorin added inline comments.Feb 12 2018, 1:28 AM
lib/AST/ASTImporter.cpp
2858

This code handles templated declarations, not template instantiations. Every TemplateDecl has an underlying templated declaration that is used while instantiating the template; it is not the same as template specialization. And it is not visible from the DeclContext it refers to via lookup. Or am I misunderstanding something?
Yes, the issue with implicit instantiations still persists but it is not the target for this patch. I have took a look at the patch; it looks like it fixes a separate issue (and I welcome you to post the patch here!).

This revision was automatically updated to reflect the committed changes.