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: > > > mizvekov wrote: > > > > ChuanqiXu wrote: > > > > > mizvekov wrote: > > > > > > ChuanqiXu wrote: > > > > > > > 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? > > > > > > Well we touch FunctionTemplates and VariableTemplates in this > > > > > > patch, because they were not doing template first. > > > > > > For whatever reason, class templates were already doing template > > > > > > first, so no need to fix that. > > > > > > > > > > > > So this patch at least puts that into consistency. > > > > > > > > > > > > Also, this patch is a pre-requisite for the template resugaring > > > > > > specialization project I am working on, and the deadline for the > > > > > > whole project is about two months from now. > > > > > > > > > > > > If I leave merging this patch until the end, it seems impossible > > > > > > that I will finish in time, as we will leave field testing this to > > > > > > the very end. > > > > > > > > > > > > So while I understand the need for a better approach, it is indeed > > > > > > putting me in an impossible situation. > > > > > > Also, this patch is a pre-requisite for the template resugaring > > > > > > specialization project I am working on, and the deadline for the > > > > > > whole project is about two months from now. > > > > > > > > > > What is the deadline you're referring? According to > > > > > https://llvm.org/docs/HowToReleaseLLVM.html, the next release branch > > > > > will be in January. > > > > > > > > > > > So while I understand the need for a better approach, it is indeed > > > > > > putting me in an impossible situation. > > > > > > > > > > I see. I understand it is bad to make perfect the enemy of better. > > > > > I'll try to give a faster response. > > > > > What is the deadline you're referring? According to > > > > > https://llvm.org/docs/HowToReleaseLLVM.html, the next release branch > > > > > will be in January. > > > > > > > > This is a GSoC that is fast becoming a GWoC, since it has been extended > > > > to the maximum possible amount of time already. > > > > > > > > > > > I see. Although GSoC projects are not guaranteed to be landed, I don't > > > want to block/object this. > > Update: when I took a look at this again. I found it break a my toy > > implementation for std modules (https://github.com/ChuanqiXu9/stdmodules). > > I reduced the failure and submit it directly at here: > > https://github.com/llvm/llvm-project/commit/1aaba40dcbe8fdc93d825d1f4e22edaa3e9aa5b1 > > since the more testing should be always good. I guess the reason may be > > that when we read the function decl, we need to defer reading its type. But > > I had no time to check. I am going to take vacation in the next 2 weeks so > > probably I can't respond quickly. > This one was actually because merging of FunctionTemplateDecls was a bit more > complicated, I made changes so that we always merge them from the > FunctionDecl side, and it's working now. I see. Thanks for the working! My work to defer reading types goes slowly. So it may not make sense to block this. But please give me a week to review the related changes. 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
