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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:623-626
+  VisitDeclaratorDecl(D);
+  Record.AddDeclarationNameLoc(D->DNLoc, D->getDeclName());
+  Record.push_back(D->getIdentifierNamespace());
+
----------------
mizvekov wrote:
> ChuanqiXu wrote:
> > ChuanqiXu wrote:
> > > ChuanqiXu wrote:
> > > > mizvekov wrote:
> > > > > ChuanqiXu wrote:
> > > > > > I still don't get the reason for the move. What's the benefit? Or why is it necessary?
> > > > > Yeah, now the type can reference the template decl, so without moving this, it can happen during import of the type that we try to read this function template bits without having imported them yet.
> > > > Oh, I guess I met the problem before (D129748 ) and I made a workaround for it (https://reviews.llvm.org/D130331). If I understood right, the patch will solve that problem. I'll check it out later.
> > > > 
> > > > (This kind of code move looks dangerous you know and I'll take a double check)
> > > After looking into the detailed change for the serialization part, I feel it is a not-so-good workaround indeed.. It looks like we need a better method to delay reading the type in the serializer. And I'm looking at it. @mizvekov would you like to rebase the series of patches to the main branch so that I can test it actually.
> > Or would it be simpler to rebase and squash them into a draft revision?
> I had given this some thought, and it made sense to me that we should deal with the template bits first, since these are closer to the introducer for these declarations, and so that it would be harder to have a dependence the other way around.
> 
> But I would like to hear your thoughts on this after you have taken a better look.
> I am working on a bunch of things right now, I should be able to rebase this on the next few days, but otherwise
> I last rebased about 4 days ago, so you can also check that out at https://github.com/mizvekov/llvm-project/tree/resugar
> That link has the whole stack, you probably should check out just the commit for this patch, as you are probably going to encounter issues with the resugarer if you try it on substantial code bases.
> It will carry other changes with it, but I think those should be safe.
I won't say it is bad to deal with template bits first. I just feel it is a workaround to avoid the circular dependent problem in deserialization. Or in another word, here the method works due to you put some decls* in the template parameter types. And we avoid the circular dependent problem by adjusting the order we deserializes. The reasons why I don't feel it is good include:
(1) Although we touched template function decl and template var decl, we don't touched template var decl. I guess it'll be a problem now or later.
(2) The solution here can't solve the similar circular dependent problem I sawed in attributes. So the method only workarounds some resulting of the same problem.

Or in one shorter explanation, it should be greater to solve the root problems. I have an idea and I am going to to do a proof-of-concept implementation first since I feel like nobody are happy about an unimplementable idea. Generally I don't like to block patches due to such reasons since it is completely not your fault but I guess it may be better to wait some time. Since if we want to allow workarounds first and clear the workarounds, things will be harder. If you want a timeline, I guess 2 months may be reasonable choices. I mean if I can't make it in 2 months and other reviewers feel this is good (what I am seeing), I feel bad to block this. (But if we're more patient, it'll be better). How do you think about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858