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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

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.

The suggested diagnostic message might not be right since the following one should be legal:

  export module m;
  ...
  module :private;
  // foo() is not declared earlier.
  inline void foo() {...}

The definition of 'foo' here should be legal although it is inline and is in private fragment.



================
Comment at: clang/test/Modules/cxx20-10-5-ex1.cpp:25-28
+export void g(X *x) {
+  fn_s();
+  fn_m();
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128328