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

[Iain Sandoe via Phabricator via cfe-commits]

iains added a comment.

the revised diagnostics look like this:

` error: {un-}exported inline function not defined before the private module fragment`

with 
` note: private module fragment begins here` pointing to the start of the PMF

If there is no PMF then we just say:

` error: {un-}exported inline function not defined `



================
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">;
----------------
iains wrote:
> 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.
> .
I think that we can consider this closed by the question to the ext reflector and the amendment proposed by core.


================
Comment at: clang/test/Modules/cxx20-10-5-ex1.cpp:25-28
+export void g(X *x) {
+  fn_s();
+  fn_m();
+}
----------------
ChuanqiXu wrote:
> vsapsai wrote:
> > Can `export inline` function call other non-`export inline` functions? Sorry if it is tested somewhere else. Curious what are the transitive restrictions, so we test edge cases.
> I guess it is not tested. But a non-export function wouldn't be exported if it is called by export function from the perspective of std. Although more tests should be fine all the time, the current test case should come from the example in the standard. We could send another test if we want.
I've changed the example to match the proposed amendment to the standard.

If you think we should have some other test case (additional to Modules/Reachability-Private), that's fine - would you like to propose one (or maybe add to an existing)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128328