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

[Iain Sandoe via Phabricator via cfe-commits]

iains marked an inline comment as done.
iains added inline comments.


================
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:
> > ChuanqiXu wrote:
> > > iains wrote:
> > > > 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.
> > > I prefer `inline function attached to a named module not defined %select{| before the private module fragment}1`. Since the `export` part is not important here and the important part is whether or not they are attached to a named module.
> > I took the error message from here:
> > https://github.com/cplusplus/draft/pull/5537
> > which was prepared after the dicussion that you started on the ext reflector.  Actually, I do not have a strong feeling either way.
> Oh, I didn't track that. I feel my suggested version is slightly better. Otherwise users might want to make it work by adding/removing `export` clause.
I removed the reference to un/exported.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128328