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

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:1618
+  QualType getSubstTemplateTypeParmType(QualType Replacement,
+                                        Decl *ReplacedDecl,
+                                        unsigned Index) const;
----------------
davrec wrote:
> ReplacedDecl -> ReplacedParamParent (see below)
We need a more apt name.

How about `ReplacedTemplateLikeEntityDecl`?


================
Comment at: clang/include/clang/AST/Type.h:4998
+  /// Gets the templated entity that was substituted.
+  Decl *getReplacedDecl() const { return ReplacedDecl; }
+
----------------
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).


================
Comment at: clang/lib/AST/Type.cpp:3709
+                                                        unsigned Index) {
+  if (const auto *TTP = dyn_cast<TemplateTypeParmDecl>(D))
+    return TTP;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858