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

[Iain Sandoe via Phabricator via cfe-commits]

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