iains added a comment. In D134267#3848974 <https://reviews.llvm.org/D134267#3848974>, @ChuanqiXu wrote:
> In D134267#3848958 <https://reviews.llvm.org/D134267#3848958>, @iains wrote: > >> In D134267#3848883 <https://reviews.llvm.org/D134267#3848883>, @ChuanqiXu >> wrote: >> >>> In D134267#3848876 <https://reviews.llvm.org/D134267#3848876>, @iains wrote: >>> >>>> If we are going to **//require//** the serialisation/deserialisation for >>>> correctness then IMO we should have an error for an incorrect compilation. >>> >>> Of course. >>> >>>> I still feel that deserialisation should do deserialisation, and Sema >>>> should be doing the diagnoses when it tries to use a decl (in fact, I >>>> would expect that to be required for correctness in the C++20 scheme, >>>> since the point of use is significant). I will defer to folks who know >>>> the history better - but this seems like a layering bug to me still. >>> >>> Another reason I forgot to mention is that the deserialization need to do >>> merging. It is common in practice that a similar declarations lives in >>> multiple module files (e.g., by GMF). Then if the deserialization needs to >>> merge things, it is naturally that the deserilizer need to do ODR checks. >>> But if you feel like the merging job belongs to Sema too, I think it might >>> not be bad/wrong. >> >> I would be concerned that doing this work in the deserialisation could: >> >> - produce false positives (diagnostics) where there should be no error until >> an object is actually used. >> - make it more difficult to track cases where what is merged at the point of >> use is semantically significant. >> >> It would seem to better that the consumer of the AST should not have to know >> whether it had been round-tripped through the serialisation/deserialization >> process. > > The reading process of modules are lazy. So the problem is relatively > mitigated. But I understand your concern in some level. (IMO) We ought to be able to move the ODR work to Sema (but I did not yet look at how hard this would be) - it seems that deserialisation should supply (lazily) decls in response to requests and the query side should determine if they are legally mergeable. However, let's move this discussion elsewhere now - I think we've covered the main points. >> From the point of view of this diff - it is relevant to consider other >> implementations that achieve the same objective - we seem to have uncovered >> some additional questions to answer. > > I feel the main blocking issue for this diff is the concern about modules > cache throw from @dblaikie and @tschuett that we (at least me) don't know the > reason. I'm still waiting for more detailed reason to understand them. > > And I feel like your opinion is mainly in the higher level. And I don't feel > it is blocking this patch. Because if we're going to refactor it actually > someday, this diff won't never be a blocker. I think that is correct - the implementations are independent (my, so far unpublished, driver patches probably would require modification, but that's not a big deal). > And under the current frame, I think this implementation is the simplest way > to achieve the same goal. So what we talked about is mainly about some higher > level things. Sure, but having a high-level plan is important too :) One specific comment about this patch; one of the key things that GCC does is to remove the need for the user to write a specific extension to use modular C++. So from that point of view, at least, this patch does not provide a solution to match the command line interfaces of GCC. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134267/new/ https://reviews.llvm.org/D134267 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
