[libcxx-commits] [PATCH] D131858: [clang] Track the templated entity in type substitution.

[David Rector via Phabricator via libcxx-commits]

davrec added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:1618
+  QualType getSubstTemplateTypeParmType(QualType Replacement,
+                                        Decl *ReplacedDecl,
+                                        unsigned Index) const;
----------------
mizvekov wrote:
> davrec wrote:
> > ReplacedDecl -> ReplacedParamParent (see below)
> We need a more apt name.
> 
> How about `ReplacedTemplateLikeEntityDecl`?
Problem is if it's a `TemplateTypeParmDecl` its not even a template like entity.  That's why I lean towards "AssociatedDecl" or something like that, that is just so vague users are forced to look up the documentation to understand it.


================
Comment at: clang/include/clang/AST/Type.h:4998
+  /// Gets the templated entity that was substituted.
+  Decl *getReplacedDecl() const { return ReplacedDecl; }
+
----------------
mizvekov wrote:
> davrec wrote:
> > To reiterate an earlier comment which may have been overlooked: I think this needs a clearer name and documentation (so long as I do not misunderstand what kinds of decls it may be - see other inline comment).  
> > 
> > Given that we have `getReplacedParameter()`, I think something like `getReplacedParamParent()` makes the most sense.
> > 
> > And the documentation should suggest the relationship between `getReplacedParameter` and this (i.e. `getReplacedParamParent()` + `getIndex()` -> `getReplacedParameter()`).  
> > 
> > Plus, add back in the original documentation for `getReplacedParameter()`, and a brief line of documentation for `getIndex()` (and do same for the Pack version below too).
> > 
> Yeah like I said, we can't suggest a relationship very much, I don't think we should try to explain how to obtain the parameter from the main entity, that would be exposing too much guts.
> 
> We just offer a method to get it, and hope for the best.
> 
> The name for this method, whatever we pick, will probably be a bit vague sounding, because in Clang a template is a very vague concept (in the philosophical sense and in the C++20 sense).
Let's say we call it `getAssociatedDecl()`.  Whatever the name I strongly think we need some good documentation explaining what these return, if they are going to be public methods.  How about something like this:
```
  /// Gets the template parameter declaration that was substituted for.
  const TemplateTypeParmDecl *getReplacedParameter() const;

  /// If the replaced TemplateTypeParmDecl belongs to a parent template/ 
  /// template-like declaration, returns that parent.
  /// Otherwise, returns the replaced TTPD.
  Decl *getAssociatedDecl() const { return AssociatedDecl; }
  
  /// If the replaced parameter belongs to a parent template/template-like      
  /// declaration, returns the index of the parameter in that parent.
  /// Otherwise, returns zero.
  unsigned getIndex() const { return SubstTemplateTypeParmTypeBits.Index; }
```




================
Comment at: clang/lib/AST/Type.cpp:3709
+                                                        unsigned Index) {
+  if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
+    return TTP;
----------------
mizvekov wrote:
> davrec wrote:
> > Will this cast ever succeed?  I.e. are there still places `Context.getSubstTemplateTypeParmType(...)` is called with a TTPD as the `ReplacedDecl`?  That would make `getReplacedDecl()` much more complicated/less understandable - can we change any such places to always pass the parent template/templated entity as the ReplacedDecl, for consistency (and so that we can rename ReplacedDecl to ReplacedParamParent)?
> Yeah, there is this one place related to concepts where we invent a substitution and there is actually no associated declaration at all, besides just the invented template parameter, so in this case the TTP is the parent and parameter.
> 
> So that is why I picked such a vaguely named acessor... because there is no order here, anarchy reigns.
I see.  So close.  I agree we need to go vague then.  I think ReplacedDecl isn't quite vague enough though; a user could easily conflate getReplacedParameter and getReplacedDecl.  I think you hit on a better term in your wording though: "AssociatedDecl".  So long as it is documented I think that could work.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D131858/new/

https://reviews.llvm.org/D131858