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