sammccall accepted this revision.
sammccall added a comment.
Nice fix! Just nits
================
Comment at: clang/include/clang/AST/TemplateName.h:197
using StorageType =
- llvm::PointerUnion<TemplateDecl *, UncommonTemplateNameStorage *,
+ llvm::PointerUnion<NamedDecl *, UncommonTemplateNameStorage *,
QualifiedTemplateName *, DependentTemplateName *>;
----------------
nit: I'd use Decl here instead of NamedDecl.
Sure, the lowest common type is NamedDecl, but that's not really important and
we don't use any of the NamedDecl interface.
================
Comment at: clang/include/clang/AST/TemplateName.h:303
+ ///
+ /// The underlying template declaration is not stored in the template name,
it
+ /// can be retrieved via the using shadow declaration.
----------------
I think this second sentence is confusing rather than helpful to understanding
to use this class (as opposed to its implementation).
The main way to get the underlying declaration is with `getAsTemplateDecl()`.
From a public POV, a Using name has a using shadow decl *and* a template decl,
the fact that we only store one of these is an implementation detail.
================
Comment at: clang/include/clang/AST/TemplateName.h:305
+ /// can be retrieved via the using shadow declaration.
+ UsingShadowDecl *getAsUsingShadowDecl() const;
+
----------------
If (after followup patches) you have a qualified name Q wrapping a using name
U, I'd expect Q.getAsUsingShadowDecl() to extract the pointer from U.storage.
I'd expect this based on the first sentence of the comment here, and also
consistent with getAsTemplateDecl() which does this sort of unwrapping.
So no changes needed if you're OK with adding that unwrapping behavior in the
next patch, otherwise I think we need to think a bit about how to set
expectations with this API.
================
Comment at: clang/lib/AST/TemplateName.cpp:86
+ if (auto *ND = Storage.dyn_cast<NamedDecl *>()) {
+ if (isa<TemplateDecl>(ND))
+ return Template;
----------------
nit: invert this so that UsingShadowDecl is the special case and TemplateDecl
the general one?
================
Comment at: clang/lib/AST/TemplateName.cpp:112
+ return TD;
+ assert(isa<UsingShadowDecl>(TemplateOrUsing));
+ return cast<TemplateDecl>(
----------------
and again here
================
Comment at: clang/lib/AST/TemplateName.cpp:256
+ //
+ // Similar to the UsingType behavior, std::vector is much more common, and
+ // provides more information in practice, we print the underlying template
----------------
I don't understand what "std::vector is much more common" there's no ground
truth for us to observe.
Rather maybe "using declarations are used to import names more often than to
export them, and using the original name is most useful in this case"?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123127/new/
https://reviews.llvm.org/D123127
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
