[PATCH] D57984: PR40642: Fix determination of whether the final statement of a statementexpression is a discarded-value expression.
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 13 13:43:26 PST 2019
rsmith added inline comments.
================
Comment at: include/clang/Parse/Parser.h:374
+ /// This context is at the top level of a GNU statement expression.
+ InStmtExpr = 0x4,
+
----------------
aaron.ballman wrote:
> It's a bit strange that the previous two enumerators are "Allow" and this is "In". Maybe it will be less of a concern when I see the uses though...
It's just a coincidence that the first two are both `Allow*` flags.
================
Comment at: include/clang/Parse/Parser.h:382
+
+ friend ParsedStmtContext operator|(ParsedStmtContext A, ParsedStmtContext B) {
+ return ParsedStmtContext((unsigned)A | (unsigned)B);
----------------
ABataev wrote:
> We have `llvm/ADT/BitmaskEnum.h`, which defines `LLVM_MARK_AS_BITMASK_ENUM` for the bitwise ops on the enums. Maybe it is better to use it rather than manually define the required operations?
That makes usage of the context a bit more verbose (`StmtCtx & X` -> `(StmtCtx & X) != ParsedStmtContext()`), but I think that's acceptable.
================
Comment at: lib/Parse/ParseStmt.cpp:1037
+ ParsedStmtContext SubStmtCtx = ParsedStmtContext::Compound;
+ if (isStmtExpr)
----------------
aaron.ballman wrote:
> ```
> ParsedStmtContext SubStmtCtx =
> ParsedStmtContext::Compound |
> (isStmtExpr ? ParsedStmtContext::InStmtExpr : 0);
> ```
> ?
There's no implicit conversion from `0` to `ParsedStmtContext`, but yeah, something like that :)
================
Comment at: lib/Parse/ParseStmt.cpp:1090-1091
+ R = handleExprStmt(Res, SubStmtCtx);
+ if (R.isUsable())
+ R = Actions.ProcessStmtAttributes(R.get(), attrs, attrs.Range);
}
----------------
aaron.ballman wrote:
> Should this be done as part of `handleExprStmt()`?
In principle that might make sense, but it'd require passing the attributes through a few extra layers, and also making sure the attributes aren't handled twice along the more-common statement parsing codepaths. I'm inclined to defer that until we have a need to have the attributes in hand when processing a full-expression (and I suspect that time may never come).
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57984/new/
https://reviews.llvm.org/D57984
More information about the cfe-commits
mailing list