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

[David Rector via Phabricator via cfe-commits]

davrec added a comment.

In D131858#3817646 <https://reviews.llvm.org/D131858#3817646>, @mizvekov wrote:

> 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.

Question was referring to @ChuanqiXu 's comment about this patch breaking modules for function importing, see his repro (which I haven't looked at closely).  I was asking if this meant that classes were already broken in a similar way, since you said this patch brings them into consistency.

>> 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.

I think we misunderstand each other again, but are you saying the breakage found by @ChuanqiXu does not need to be fixed because it is out of scope?  (If classes are already broken due to this issue, the argument might have some validity.)

I wouldn't feel comfortable accepting this without either a) a hack to fix the breakage or b) @ChuanqiXu saying it is okay.  Maybe best to punt until @ChuanqiXu gets back from vacation in a couple weeks IIUC?



================
Comment at: clang/include/clang/AST/Type.h:5060
+  /// Gets the template parameter declaration that was substituted for.
+  const TemplateTypeParmDecl *getReplacedParameter() const;
+
----------------
mizvekov wrote:
> 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.
I see, so it is a necessary change, and anyone who really depends on this returning the TTPT will need to convert it via `Context.getTemplateTypeParmType(...)`.  But any code written getReplacementParameter()->getDecl() would just lose the getDecl() and be fine.  And since all the info is in decl most people will only need to do the latter.  Given the necessity I think that's okay.


================
Comment at: clang/include/clang/Sema/Template.h:80
+    struct ArgumentListLevel {
+      Decl *AssociatedDecl;
+      ArgList Args;
----------------
mizvekov wrote:
> 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...
The reason we need this unfortunately vague term "AssociatedDecl" in STTPT is because it can either be a template/template-like declaration *or* a TemplateTypeParmDecl.  But here in MLTAL, it won't be a TTPD, will it?  It will always be the parent template/template-like declaration, right?  So there is no need for vagueness.  `ReplacedDecl` or `ParentTemplate` or something like that seems more appropriate.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858