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

[David Rector via Phabricator via libcxx-commits]

davrec added a comment.

This looks good, except for the confusion around the name `getReplacedDecl` - I would really like the public AST methods to be clearly named and documented, so please address that.

Thanks for doing the performance tests.  It may create some additional unique STTPTs, but not to do with pack expansion, i.e. not to the extent that created problems for D128113 <https://reviews.llvm.org/D128113>, so I think performance concerns have been addressed.

And, it seems possible that the additional info added to STTPT here might make D128113 <https://reviews.llvm.org/D128113> (storing pack index or introducing a new sugar type to help infer it) unnecessary for resugaring purposes - I will add an inline comment to https://reviews.llvm.org/D127695 to show you what I mean/ask about this.

(I'm tempted to hit accept but a. I think there is at least one other issue raised by another reviewer which hasn't been addressed and b. I'm unsure of the etiquette and don't want to step on toes particularly given the recent changes to code ownership.)



================
Comment at: clang/include/clang/AST/ASTContext.h:1618
+  QualType getSubstTemplateTypeParmType(QualType Replacement,
+                                        Decl *ReplacedDecl,
+                                        unsigned Index) const;
----------------
ReplacedDecl -> ReplacedParamParent (see below)


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



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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858