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