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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
