ChuanqiXu 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">; ---------------- 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. ================ Comment at: clang/lib/Sema/SemaDecl.cpp:9701 + // have a definition at that point. + if (isInline && !D.isFunctionDefinition() && getLangOpts().CPlusPlus20 && + NewFD->hasOwningModule() && ---------------- ================ Comment at: clang/test/Modules/cxx20-10-5-ex1.cpp:25-28 +export void g(X *x) { + fn_s(); + fn_m(); +} ---------------- iains wrote: > 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)? The current one looks fine to me. 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
