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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to