ChuanqiXu 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: > 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()`. I think we need to touch `diagnoseQualifiedDeclaration()` after all. Since there is a potential risk of crashing in it. If we didn't fix it in `diagnoseQualifiedDeclaration()` and find other place to fix the issue, I think it may crash potentially one day in the process of developing or we might ignore some paths to diagnoseQualifiedDeclaration()`. It would be a disaster. And you said "there's no declaration there *to* export". And I noticed that there is error/warning in `Sema::ParsedFreeStandingDeclSpec` which would emit this kind of error/warning. But as the title describes, it only works for free standing declaration, which is not the same with the issue in bug47715. And `diagnoseQualifiedDeclaration()` would handle the qualified redeclaration. So it looks a good place to me. BTW, I found the current patch could handle [module.interface]/p6 partially for qualified redeclaration surprisingly. See the newly added test case for example. Finally, given that we need to touch `diagnoseQualifiedDeclaration()` to fix the crash, I think the patch should be good and we could try to cover [module.interface]/p6 in successive patches. Do you think so? ================ 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(); ---------------- aaron.ballman wrote: > 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. BYW, I added a new issue for the example in [module.interface]p6 here: https://bugs.llvm.org/show_bug.cgi?id=52395. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112903/new/ https://reviews.llvm.org/D112903 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
