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

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

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?

In D128328#3603781 <https://reviews.llvm.org/D128328#3603781>, @vsapsai wrote:

> From the perspective of handling `err_export_inline_not_defined` error as a developer what about the following option?
>
>   export inline void fn_e(); // note: function 'fn_e' exported as 'inline' here
>   
>   // ...
>   module :private;
>   void fn_e() {}  // error: definition of function 'fn_e' can not be inlined because it is private
>
> My suggestion isn't about specific wording but about emitting an error for the definition and mention declaration in the note. Will it make easier to explain the situation? Because I'm not a fan of "must be defined within the module purview" and don't know how digestible it will be for others. Please note that the suggestion is purely from user perspective, I haven't checked how it might affect the implementation.

I agree my error message is kinda "implementor-speak" really, there's a tension between using something that will allow the user to find the source of the problem with a google search and and avoiding that.

We could implement what you suggest (pending a resolution of what we're actually supposed to be implementing) - I guess - but we'd need to defer the check until the end of the TU - i.e. after any potential PMF.  I think we can differentiate the case that @ChuanqiXu noted (definition local to the PMF) because that would not have an entry in the PendingInlineExports table.

As for transitive cases, I agree we need to defer consideration of that until we can discuss with core - otherwise this patch will become a rabbit hole ;)



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11155
+def err_export_inline_not_defined : Error<
+  "exported inline functions must be defined within the module purview"
+  " and before any private module fragment">;
----------------
ChuanqiXu wrote:
> iains wrote:
> > iains wrote:
> > > ChuanqiXu wrote:
> > > > From my reading, 'exported' is not emphasized.
> > > it is here:
> > > https://eel.is/c++draft/module#private.frag-2.1
> > > ( I agree it is somewhat confusing, but the export makes the linkage external, which the example treats differently from the fn_m() case which has module linkage).
> > > 
> > > It is possible that we might need to pull together several pieces of the std and maybe ask core for clarification?
> > > it is here:
> > > https://eel.is/c++draft/module#private.frag-2.1
> > > ( I agree it is somewhat confusing, but the export makes the linkage external, which the example treats differently from the fn_m() case which has module linkage).
> > 
> > hmm... my linkage comment is wrong - however the distinction between exported and odr-used seems to be made here (fn_m() and fn_e()).
> > > 
> > > It is possible that we might need to pull together several pieces of the std and maybe ask core for clarification?
> > 
> > 
> What I read is:
> > [dcl.inline]p7: https://eel.is/c++draft/dcl.inline#7
> >     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.
> 
> and the definition of `definition domain` is:
> > [basic.def.odr]p12: https://eel.is/c++draft/basic#def.odr-12
> >       A definition domain is a private-module-fragment or the portion of a translation unit excluding its private-module-fragment (if any).
> 
> The definition of "attached to a named module" is:
> > [module.unit]p7: https://eel.is/c++draft/module.unit#7
> >      A module is either a named module or the global module. A declaration is attached to a module as follows: ...
> 
> So it is clearly not consistency with [module.private.frag]p2.1. I would send this to WG21.
Yes, that was what I found - maybe we are  missing something about the export that changes those rules.
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128328