aaron.ballman added reviewers: urnathan, hubert.reinterpretcast. 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< ---------------- 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. 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
