ChuanqiXu requested review of 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< ---------------- ChuanqiXu wrote: > 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. @aaron.ballman @urnathan I have sent a draft to reject the codes before `diagnoseQualifiedDeclaration()`. The draft is here: https://reviews.llvm.org/D113239. The draft is not completed to review the details. The key idea is to insert checks for `ExportDecl` on the fly to construct the AST. Due to the complexity of semantics analysis , we might need to insert the checks many times. It may not be too terrible since I found many `Check*` functions in `Sema`. So may be this is what we used to do. And the draft could reject the example in https://eel.is/c++draft/module.interface#6. It looks like not so hard as I imaged. BTW, I have tried to let the compiler to emit "declaration does not declare anything" when it sees "export template <typename T> X<T>::Y". But I failed. It is not easy for me. And I found that the compiler wouldn't emit "declaration does not declare anything" when the declaration is in a `DeclContext`. Here is an example: https://godbolt.org/z/3j63PnTc7. So the compiler used to handle this case in `diagnoseQualifiedDeclaration()`. Personally, it looks not bad to handle `export template<> X::Y` in `diagnoseQualifiedDeclaration()` at least from the names. And I am wondering if we could export a non-namespace-scope name without using qualifiers? If no, it looks better for me to handle this in `diagnoseQualifiedDeclaration()`. BTW, the following example is **not** what I meant: ``` struct X { struct Y {}; }; export using Z = typename X::Y; ``` Finally, it looks like we need to touch `diagnoseQualifiedDeclaration()` after all. Otherwise, it would crash for that example. 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
