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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
