[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

[Chuanqi Xu via Phabricator via cfe-commits]

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