urnathan 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: > 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. 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
