ChuanqiXu planned changes to this revision. 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< ---------------- urnathan wrote: > aaron.ballman wrote: > > ChuanqiXu wrote: > > > 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? > > > I think we need to touch diagnoseQualifiedDeclaration() after all. Since > > > there is a potential risk of crashing in it. > > > > My point is that we may have wanted to reject this code before ever needing > > to call `diagnoseQualifiedDeclaration()` in the first place. aka, it might > > be just as valid to assert we never see an `ExportDecl` here because the > > caller should have already handled that case. > > > > > 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. > > > > The declaration is invalid in C++20 because it does not declare anything > > (this is the same example with the export keywords removed): > > https://godbolt.org/z/8zc9q7fno > > > > Clang fails the first one because we don't yet implement P0634R3, but the > > presence of the export keyword should not change diagnostic behavior here. > > > > > Agreed, 'export' is only applicable to namespace-scope declarations. We > should reject it applying to non-namepace-scope entities. Got it. It is important to make the code semantics consistent with the spec. 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
