Page MenuHomePhabricator

[clang] Fix returning the underlying VarDecl as top-level decl for VarTemplateDecl.
ClosedPublic

Authored by hokein on Oct 9 2020, 12:27 AM.

Details

Summary

Given the following VarTemplateDecl AST,

VarTemplateDecl col:26 X
|-TemplateTypeParmDecl typename depth 0 index 0
`-VarDecl X 'bool' cinit
  `-CXXBoolLiteralExpr 'bool' true

previously, we returned the VarDecl as the top-level decl, which was not
correct, the top-level decl should be VarTemplateDecl.

Diff Detail

Event Timeline

hokein created this revision.Oct 9 2020, 12:27 AM
hokein requested review of this revision.Oct 9 2020, 12:27 AM
sammccall accepted this revision.Oct 9 2020, 1:09 AM

Nice catch, LG with readability nits

clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
119

(you could fold this into the above test if you want)

124

Is WithTemplateArgs here testing what you want (args vs params)?
Seems like it would be clearer to assert on DeclKind

clang/lib/Parse/ParseDecl.cpp
2212–2214

The purpose of the renaming is less than obvious here.

Can we rename VarTemplateDecl to OuterDecl or so? (And change the type to Decl)

This revision is now accepted and ready to land.Oct 9 2020, 1:09 AM
hokein updated this revision to Diff 297512.Oct 12 2020, 1:45 AM
hokein marked 3 inline comments as done.

address review comments.

This revision was landed with ongoing or failed builds.Oct 12 2020, 2:06 AM
This revision was automatically updated to reflect the committed changes.
hokein added a subscriber: rsmith.Oct 12 2020, 6:31 AM

Is this https://github.com/clangd/clangd/issues/554 ? :-)

Yeah, the github issue exposes multiple bugs, this is part of the fix.

The AST of VarTemplateSpeicalizationDecl is a bit unusual, given the follow code,

template <typename> bool X = true;
bool Z = X<int>;
TranslationUnitDecl 0x8a2ec28 <<invalid sloc>> <invalid sloc>
|-VarTemplateDecl 0x8a6faf8 </tmp/t.cpp:1:1, col:30> col:26 X
| |-TemplateTypeParmDecl 0x8a6f9c0 <col:11> col:19 typename depth 0 index 0
| |-VarDecl 0x8a6fa90 <col:21, col:30> col:26 X 'bool' cinit
| | `-CXXBoolLiteralExpr 0x8a6fb98 <col:30> 'bool' true
| `-VarTemplateSpecializationDecl 0x8a6fd08 <col:21, col:30> col:26 used X 'bool' cinit
|   |-TemplateArgument type 'int'
|   | `-BuiltinType 0x8a2ed20 'int'
|   `-CXXBoolLiteralExpr 0x8a6fb98 <col:30> 'bool' true
|-VarDecl 0x8a6fbb8 <line:2:1, col:15> col:6 Z 'bool' cinit
| `-ImplicitCastExpr 0x8a6ff28 <col:10, col:15> 'bool' <LValueToRValue>
|   `-DeclRefExpr 0x8a6fed8 <col:10, col:15> 'bool' lvalue VarTemplateSpecialization 0x8a6fd08 'X' 'bool'
`-VarTemplateSpecializationDecl 0x8a6fd08 <line:1:21, col:30> col:26 used X 'bool' cinit   <---- here
  |-TemplateArgument type 'int'
  | `-BuiltinType 0x8a2ed20 'int'
  `-CXXBoolLiteralExpr 0x8a6fb98 <col:30> 'bool' true

Note that the implicitly-instantiated VarTemplateSpecializationDecl is one of TranslationUnitDecl::decls(), this is different than function/class templates -- for function/class templates, only *explicit* template specializations will be in the TranslationUnitDecl::decls()

Related code is at https://github.com/llvm/llvm-project/blob/master/clang/lib/Sema/SemaTemplateInstantiateDecl.cpp#L5015. @rsmith do you think this is expected? or we should fix that? like making it align with function/class templates?

nridge added a subscriber: nridge.Oct 12 2020, 4:33 PM

I posted about this a while ago, but wasn't sure how to fix. Thanks for fixing!