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

[Matheus Izvekov via Phabricator via libcxx-commits]

mizvekov added a comment.

In D131858#3815057 <https://reviews.llvm.org/D131858#3815057>, @davrec wrote:

>> So this patch at least puts that into consistency.
>
> Is there already an analogous breakage for classes then?

I am not sure I follow the question, but I meant that classes were already deserealizing templates first, so no need to fix them.

> Given @mizvekov's approaching deadline and @ChuanqiXu's limited availability, I'm inclined to think if @mizvekov can hack together a reasonable fix for this breakage, leaving a more rigorous/general solution for later as @ChuanqiXu suggests he would like to pursue, that would be sufficient to land this.  Does anyone disagree?

Well I wouldn't say this fix is a hack any more than the whole code is, as it's already playing by the rules of the existing implementation.

We already have to deal with objects being accessed before they are fully deserialized, this is part of the design of the current solution.
The present patch just adds a, presumably new, case where the order is important.

Any changes in order to have a more strict, atomic deserialization are out of scope here.

By the way, I will let this patch sit for another week, as it will need some significant rebasing against a recent change that was merged, which could also possibly be reverted.



================
Comment at: clang/include/clang/AST/Type.h:5060
+  /// Gets the template parameter declaration that was substituted for.
+  const TemplateTypeParmDecl *getReplacedParameter() const;
+
----------------
davrec wrote:
> @sammccall , drafting you as a representative for downstream AST users, do you have any objection to changing this return type to a `TemplateTypeParmDecl` from a `TemplateTypeParmType`?  IIUC the change is not strictly necessary but is more for convenience, correct me if I'm wrong @mizvekov.  But I'm inclined to think STTPT is not much used downstream so the churn will be minimal/acceptable, am I wrong?
I don't think this is strictly true.

We don't need to store the type, as that would increase storage for no benefit. The type won't have more usable information than the associated parameter declaration. The type can support a canonical representation, which we don't need.

We will store the template-like declaration + index, and simply access the parameter decl from there.
Otherwise creating a type from that requires the ASTContext and would possibly involve allocation.


================
Comment at: clang/include/clang/Sema/Template.h:80
+    struct ArgumentListLevel {
+      Decl *AssociatedDecl;
+      ArgList Args;
----------------
davrec wrote:
> Actually I think this one should be changed back to `ReplacedDecl` :)
> ReplacedDecl makes perfect sense in MLTAL, AssociatedDecl seems to make better sense in STTPT.
I would be against introducing another term to refer to the same thing...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858