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