ChuanqiXu9 wrote: > > > I'd feel uncomfortable exposing a `noload_redecls` interface because it > > > can potentially break the AST invariants. I think allowing non-modules > > > developers to choose when to complete a redeclaration chain will > > > introduce more bugs than it can fix ;) > > > I'd propose to move forward with `getMostRecentExistingDecl` if it does > > > the job. We can have a separate PR discussing the ergonomics if there is > > > a strong feeling about going this way. > > > > > > I'd like it as: (1) We already have `noload_lookup` and other similar > > "unpublic" API (e.g., `getOwningModuleID` `invalidateCachedLinkage`) in > > DeclBase.h. > > Well, sure, `noload_lookup` is done to check what's the current state before > completing something externally. In fact, I think we should keep these > interfaces at minimum and only use them where we really have no other way out.
I feel it sounds over nervous. It is not that bad. And the interfaces will look very consistency. We add the interface doesn't mean we ask programmers to use that. It is simply an interface doing the natural things. > > > (2) Merging Redecls is a major performance hurting point of using modules. > > Only when the modules approach is not bottom up. The non-bottom up approach > has bigger problems than performance at the moment and frankly we cannot > reasonably support it right now... Even with the bottom up approach, we may face the merging problems due to generated decls (e.g, template instantiations) but just mitigates the problem. And for C++20 named modules, if we don't export macros (which a lot of people are not in favor), the user may have to face that problem in practice. For that I sent https://discourse.llvm.org/t/request-for-discussion-clang-a-bidirectional-decl-query-system/88945 and `noload_decls` is necessary for that. > > > (3) In the past, there are many "fix" which just tried to mitigate the bug > > by loading all redecls without understanding the real problem. > > Does that need to happen in this PR? I believe it brings down the peak memory > significantly for most of the module users and we should not wait too long to > get this merged. If the testing results shows the current failure in google side shows it is needed, yes it is needed for the PR. Otherwise, it is not. https://github.com/llvm/llvm-project/pull/133057 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
