[PATCH] D112903: [C++20] [Module] Fix front end crashes when trying to export a type out of a module

Wed Nov 3 05:52:35 PDT 2021

aaron.ballman added inline comments.

================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7784-7785
   "because namespace %1 does not enclose namespace %2">;
+def err_invalid_declarator_in_export : Error<"cannot export %0 here "
+  "because it had be declared in %1.">;
 def err_invalid_declarator_global_scope : Error<
----------------
aaron.ballman wrote:
> I think this diagnostic text is more clear based on the standards text you cited. This would also come with a note diagnostic to point to the previous declaration.
Given that this is not intended for p6 specifically, I think my suggestion is incorrect. But I am also not certain your diagnostic is either, but it's really hard to tell given where our current support is for modules. All of the other compilers suggest that an unqualified id is expected to be found, and I tend to agree -- there's no declaration there *to* export, just the type specifier for a declaration. This makes me think the issue is elsewhere and perhaps we shouldn't even be getting into `diagnoseQualifiedDeclaration()`.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:5748-5750
+    else if (isa<ExportDecl>(Cur))
+      Diag(Loc, diag::err_invalid_declarator_in_export)
+          << Name << cast<NamedDecl>(DC) << SS.getRange();
----------------
ChuanqiXu wrote:
> aaron.ballman wrote:
> > I don't believe this is sufficient to cover [module.interface]p6. I tried out the example from the paragraph in the standard and we still silently accept it.
> Yes, this patch is not intended to cover [module.interface]p6. The intention is to fix the crash since I feel it would take a longer time to support [module.interface]p6. And crash is not good any way. I plan to support [module.interface]p6 in successive patches. Do you happy with this?
> BTW, I have a plan to support clang's c++20 module to a workable state. And I am working on the reachable definition in https://eel.is/c++draft/module.reach#3 and  https://eel.is/c++draft/module.global.frag#3.
Thank you for the clarification! Fixing the crash is definitely a step in the right direction.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112903