hokein added inline comments.
================
Comment at: clang/include/clang/AST/PropertiesBase.td:666
}
- def : Property<"declaration", TemplateDeclRef> {
+ def : Property<"templateDecl", TemplateDeclRef> {
let Read = [{ qtn->getTemplateDecl() }];
----------------
sammccall wrote:
> this seems to be flattening the TemplateName into its possible attributes.
>
> Is it possible to encode it as a single TemplateName property? are there
> particular advantages/disadvantages?
use `TemplateName` now.
================
Comment at: clang/include/clang/AST/TemplateName.h:418
+ /// The underlying template name, it is either
+ // 1) a Template TemplateName -- a template declaration or set of overloaded
+ // function templates that this qualified name refers to.
----------------
sammccall wrote:
> sammccall wrote:
> > I'm confused about the overloaded case: before this patch it was a single
> > TemplateDecl. Have we actually added the possibility that
> > UnderlyingTemplate is an Overload in this patch? Where?
> nit: "Template TemplateName" reads a little strangely or reminds of
> TemplateTemplateParm
> I think "a Template" is clear in context. (and below)
> before this patch it was a single TemplateDecl. Have we actually added the
> possibility that UnderlyingTemplate is an Overload in this patch? Where?
No, this was basically moved from the original comment, but you're right, the
comment doesn't reflect the current code, I think it was oversight when we
abstracted out a QualifiedTemplateName in
https://github.com/llvm/llvm-project/commit/d28ae27d8d2e20672e182710160f27cf821b55fd.
Removed it.
================
Comment at: clang/include/clang/AST/TemplateName.h:444
+ /// declaration, returns the using-shadow declaration.
+ UsingShadowDecl *getUsingShadowDecl() const {
+ return UnderlyingTemplate.getAsUsingShadowDecl();
----------------
sammccall wrote:
> It seems all of the callers of this function ultimately just use it to
> reconstruct the underlying template name. This makes sense, because you
> mostly don't look at QualifiedTemplateName unless you're handling every
> TemplateName case, and normally handle it by recursion.
>
> I think:
> - we should definitely expose the underlying TemplateName and many callers
> should use that
> - we should probably remove getUsingShadowDecl() unless there are enough
> callers that benefit from not getting it via TemplateName
> - we should *consider* removing getTemplateDecl(), it makes for a simpler
> logical model (basically the top TemplateName::getTemplateName() decl does
> this smart unwrapping of sugar, but the specific representations like
> QualifiedTemplateName just expose their components). If it's too much of a
> pain for callers, we don't need to do this of course.
> we should definitely expose the underlying TemplateName and many callers
> should use that
+1, this sounds good, and it can also simplify some user code.
> we should *consider* removing getTemplateDecl()
This is doable, there are not too many callers of this API (~14 in-tree
occurrences), but I'd prefer to do it in a follow-up patch.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123775/new/
https://reviews.llvm.org/D123775
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
