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?
================
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@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
