[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.

[Richard Smith via cfe-commits]

On Tue, 12 Feb 2019, 05:28 Aaron Ballman via Phabricator via cfe-commits, <
cfe-commits at lists.llvm.org> wrote:

> aaron.ballman added a comment.
>
> Considering that this has been fertile ground for buggy edge cases, should
> we revert my original changes from the 8.0 branch to give this more time to
> bake on trunk before releasing? Or do you think this is fine to move onto
> the release branch once finalized?
>

Let's revert your change for 8.0 and fix forward on trunk.

================
> Comment at: include/clang/AST/Stmt.h:1598-1602
> +  const Expr *getExprStmt() const;
> +  Expr *getExprStmt() {
> +    const ValueStmt *ConstThis = this;
> +    return const_cast<Expr*>(ConstThis->getExprStmt());
> +  }
> ----------------
> rsmith wrote:
> > aaron.ballman wrote:
> > > We typically implement this in reverse, where the non-const version
> holds the actual implementation and the const version performs a
> `const_cast`.
> > We do. Do you think that's preferable? I like that this way around
> proves that the `const` version is const-correct, but it's a tiny benefit
> and I'm fine with just being consistent.
> Personally, I prefer the way you have it here (though I wish we had a
> global helper function to hide the dispatch dance).
>
>
> ================
> Comment at: include/clang/Parse/Parser.h:374
> +  /// a statement expression and builds a suitable expression statement.
> +  StmtResult handleExprStmt(ExprResult E, WithinStmtExpr IsInStmtExpr);
>
> ----------------
> rsmith wrote:
> > aaron.ballman wrote:
> > > Rather than passing around `IsInStmtExpr` to a bunch of parser APIs,
> would it make more sense to add an RAII object that sets some state on
> `Parser` to test it? The proliferation of arguments seems unfortunate given
> how much of a corner case statement expressions are.
> > Yeah, I tried that approach first, but the parser state turns out to be
> much worse, because it puts a burden on every form of statement that
> constructs a nested parsing context to reset that state. We can put the
> resetting on `ParseScope`, but it still needs to have an effect in the case
> where the scope is disabled, which is surprising, and there's no guarantee
> such statement constructs that can end in an expression will have a nested
> scope (consider, for instance, constructs like `return x;` or `goto
> *label;`). This is really local state that should only be propagated
> through a very small number of syntactic constructs rather than global
> state.
> >
> > Maybe we could combine the new flag with the `AllowOpenMPStandalone` /
> `AllowedConstructsKind` flag into a more general kind of "statement
> context". The propagation rules aren't quite the same
> (`AllowOpenMPStandalone` doesn't get propagated through labels whereas
> `IsInStmtExpr` does), which is a little awkward, but maybe that's not so
> bad -- and maybe that's actually a bug in the OpenMP implementation?
> > Maybe we could combine the new flag with the AllowOpenMPStandalone /
> AllowedConstructsKind flag into a more general kind of "statement context".
>
> That seems like a sensible approach to me.
>
>
> Repository:
>   rC Clang
>
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D57984/new/
>
> https://reviews.llvm.org/D57984
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190212/845d4ad7/attachment.html>