cor3ntin planned changes to this revision.
cor3ntin added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:553
+def err_expected_alias_after_using_in_init_statement : Error<
+ "expected alias declaration after using in init statement">;
+def ext_alias_in_init_statement : ExtWarn<
----------------
rsmith wrote:
> aaron.ballman wrote:
> > This diagnostic confuses me. An init-statement can be an
> > expression-statement, simple-declaration, or alias-declaration. So if we're
> > in an init-statement and see a `using` keyword, the only think we can be
> > parsing is an alias declaration, right?
> >
> > I guess I would have expected that we'd eventually wind up calling
> > `Parser::ParseAliasDeclarationAfterDeclarator()` via
> > `ParseUsingDeclaration()` and that would handle any parsing-related
> > concerns with the construct.
> >
> > Beyond that -- users don't really know what an init statement is, I think
> > we'd usually call this a "condition" instead of an init statement in
> > diagnostics.
> I think there's a more fundamental problem with this diagnostic: it's a
> compiler-oriented explanation of what went wrong *for us*, not a
> user-oriented explanation of what's wrong *with their code*. If the user
> wrote `if (using X::A; A a = b)` or `if (using namespace N; ...)`, telling
> them that they could have written an alias declaration instead is not helpful
> -- that has no relevance to the problem they're trying to solve. It'd be
> better to say "using [namespace] declaration cannot appear here" and not
> mention alias declarations at all.
We do but `ParseUsingDeclaration` can parse other things, which are not allowed
there.
I will try to modify `ParseUsingDeclaration`, which might not lead to better
diagnostics, but at least won't be compiler-oriented.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:555
+def ext_alias_in_init_statement : ExtWarn<
+ "alias declaration in init statements is a C++2b extension">,
+ InGroup<CXX2b>;
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Same concern here about "init statements", we probably should list the
> > constructs specifically.
> >
> > Also, I think we're missing the "is incompatible with standards before"
> > variant of this extension diagnostic.
> We could use a `%select` to specify the context instead of saying "init
> statement", but actually describing the contexts seems hard to do succinctly
> and clearly. In this case I think it'd be OK to just say "in this context" or
> similar if we don't want to mention init-statements. (As far as I can tell,
> literally no-one outside a C++ parser's test suite has ever put a `typedef`
> in an init-statement, so we should similarly assume this feature will never
> be used outside our own tests. The `%select` is probably not worth the
> effort.)
This was my justification for not doing more work. I agree that "in this
context" is better
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1924-1928
+ const Decl *InitDecl = DG.get().getSingleDecl();
+ if (!isa<TypeAliasDecl>(InitDecl)) {
+ Diag(DeclStart, diag::err_expected_alias_after_using_in_init_statement)
+ << SourceRange(DeclStart, DeclEnd);
+ }
----------------
aaron.ballman wrote:
> Okay, to verify my understanding of the logic here: you call
> `ParseUsingDeclaration()`, but that may parse a using declaration or an alias
> declaration, so you need to handle the case where it parsed something that's
> not allowed.
>
> Assuming that's correct, I think a better approach is for
> `ParseUsingDeclaration()` to pay attention to the `DeclaratorContext` it is
> given, and when it's an init statement, refuse to parse anything other than
> an alias declaration. Then you can get rid of
> `err_expected_alias_after_using_in_init_statement` entirely, as the user will
> get the typical parsing errors. If those diagnostics are low quality and we
> need a custom diagnostic instead, that logic can live in
> `ParseUsingDeclaration()` instead.
>
> That just about obviates the need for
> `ParseAliasDeclarationInInitStatement()` entirely, but this function still
> adds value because of the extension/precompat warnings so I think it's worth
> keeping around.
Yes, I considered modifying `ParseUsingDeclaration` but wanted to keep the
change somewhat localized, and keep `ParseUsingDeclaration` as is on the basis
that no other place in the standard only accept alias-declaration. But the
solution you suggest is certainly possible.
I don't know how much better the diagnostic would be.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D111175/new/
https://reviews.llvm.org/D111175
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
