mizvekov 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());
+
----------------
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.


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

Reply via email to