[clang] [Serialization] Load Specializations Lazily (PR #76774)

Chuanqi Xu via cfe-commits cfe-commits at lists.llvm.org
Sun Feb 18 01:41:01 PST 2024


ChuanqiXu9 wrote:

> > > > > > > > > > [do not merge] [runtime-cxxmodules] Rework our lazy template specialization deserialization mechanism [root-project/root#14495](https://github.com/root-project/root/pull/14495)
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > From [root-project/root#14495](https://github.com/root-project/root/pull/14495), I see there is new reply saying the testing is actually fine. Do you think we still need to split the patch?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > That comment was concerning the version of the patch that had the lazy template deserialization turned off by default. Yes, I still think that this patch should implement tha on-disk hash table on top of D41416
> > > > > > > 
> > > > > > > 
> > > > > > > OK. And would you like to send a PR for D41416? I've already fixed the issue mentioned in the review page. Then I'd like to send small and incremental patches on that.
> > > > > > 
> > > > > > 
> > > > > > Do you mean that I should open a PR for D41416 and you will apply your patch there? I have no problem if we do everything here as part of this PR. This way we will have the full history of how this was born in one place ;)
> > > > > 
> > > > > 
> > > > > Yeah, and please create a branch under llvm/llvm-project directly. Then I can perform stacked PR on that.
> > > > 
> > > > 
> > > > There it is: https://github.com/llvm/llvm-project/tree/users/vgvassilev/D41416_D153003
> > > 
> > > 
> > > If I drop it then our tests will break. IIUC that's somewhere deep in the hasher and should be not impact this PR. Does this make the work on the on-disk hashtable more complicated in some way?
> > 
> > 
> > No, it won't block the work for on-disk hashtable. But if we want to land that, we must understand what happened actually...
> 
> We can’t land that without attaching your on-disk hashtable implementation part of this PR because of what’s mentioned here [#76774 (comment)](https://github.com/llvm/llvm-project/pull/76774#issuecomment-1893222165)

I know that. But we're not talking about the same thing. This is one of the reason that we can't land that. But my point is that we can't land that if we don't understand what's going wrong without that patch.

https://github.com/llvm/llvm-project/pull/76774


More information about the cfe-commits mailing list