ChuanqiXu added a comment. 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. > 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 to 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. 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. 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
