[PATCH] D128328: [C++20][Modules] Improve handing of Private Module Fragment diagnostics.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D128328#3603967 <https://reviews.llvm.org/D128328#3603967>, @iains wrote:

> In D128328#3603953 <https://reviews.llvm.org/D128328#3603953>, @ChuanqiXu wrote:
>
>> In D128328#3603945 <https://reviews.llvm.org/D128328#3603945>, @iains wrote:
>>
>>> In D128328#3603942 <https://reviews.llvm.org/D128328#3603942>, @ChuanqiXu wrote:
>>>
>>>> In D128328#3603940 <https://reviews.llvm.org/D128328#3603940>, @iains wrote:
>>>>
>>>>> In D128328#3602646 <https://reviews.llvm.org/D128328#3602646>, @iains wrote:
>>>>>
>>>>>> In D128328#3601080 <https://reviews.llvm.org/D128328#3601080>, @ChuanqiXu wrote:
>>>>>>
>>>>>>> It looks like we need to handle inline variable as well to match the intention.
>>>>>>
>>>>>> can you construct a test-case, where this would apply and which is not already diagnosed as incorrect?
>>>>>
>>>>> Did you have some ideas here?
>
>
>
>> I feel like the following one should be the test case:
>>
>>   export module A;
>>   [export] inline int a;
>>
>> Here the inline variable 'a' is declared in the definition domain but not defined. This violates [dcl.inline]p7:
>>
>>> If an inline function or variable that is attached to a named module is declared in a definition domain, it shall be defined in that domain.
>
> hmm ... isn't that implicitly initialised with 0?

Oh, I just realized every declaration is a definition too except some cases: https://eel.is/c++draft/basic.def#2

And I failed to give an example that a undefined inline variable declaration attached to the named modules. I wondered:

  export module A;
  extern "C++" inline int a;
  extern "C++" int a = 0;

But now 'a' is attached to global module instead of named modules. So I just failed to find a good example... very sorry for wasting the time

>> Also, **if** [module.private.frag]p2.1 is changed into:
>>
>>> the point by which the definition of an [exported] inline function or variable is required
>>
>> The test above would cover this too.
>>
>> BTW, it shows we could lack test like:
>>
>>   export module A;
>>   [export] inline void func(); // no definition in the definition domain
>
> I think the current impl. should catch that - the only difference would be that, in the case there's a definition in the PMF, there would be a note about the unreachable definition.

It looks like the current impl doesn't catch this: https://godbolt.org/z/fh9Ehfdj5 . I think I don't make mistake this time since a function declaration without function body shouldn't be a definition.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128328/new/

https://reviews.llvm.org/D128328