[PATCH] D110482: [clang] Implement if consteval (P1938)

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseStmt.cpp:1520-1525
+  if (IsConsteval) {
+    if (!isa_and_nonnull<class CompoundStmt>(ThenStmt.get())) {
+      Diag(ConstevalLoc, diag::err_expected_after) << "consteval"
+                                                   << "{";
+      return StmtError();
+    }
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > aaron.ballman wrote:
> > > cor3ntin wrote:
> > > > aaron.ballman wrote:
> > > > > Oh. I see where the diagnostic is happening. I think it's unclean to parse a statement and then diagnose it later as being the wrong kind of statement -- why not parse the correct kind of statement initially?
> > > > I think the code reads better that way - we need to parse any kind of statement in other cases. In any case the work is about the same
> > > Hmmm, this makes me a bit uncomfortable, but I'm not convinced it's a problem either. Calling `ParseCompoundStmt()` would require checking the token first to avoid an assert, so I can understand better wanting to avoid the messier code that comes from that. However, calling the correct parsing method also means that we don't attempt to parse something we shouldn't parse only to reject it much later; I'm a bit concerned that we'll get some bad error recovery when parsing the more general thing. However, my attempts to come up with test cases where this matters are so far falling pretty flat. One test that would help (and should pass still) is:
> > > ```
> > > if consteval ({1;}); // Do we correctly reject the statement expression?
> > > ```
> > Please hold off on changes here for the moment, as I think I may have identified a DR. Consider code like:
> > ```
> > if consteval [[]] {
> > }
> > ```
> > According to the C++ grammar, this construct should not be accepted. However, that's inconsistent with every other form of if statement and seems like a defect to me. I've asked on the Core reflectors to see.
> As discussed offline I'm letting this as is so that we can parse attributes
The sentiment I'm getting from the Core reflectors is that this is an oversight and there's no reason we shouldn't support attributes here.

So I think the current form is good, but I'd like some parsing test coverage for attributes:
```
// Test in test/Parser/
if consteval [[]] {
} else [[]] {
}

if !consteval [[]] {
} else [[]] {
}
```
and some AST test coverage that shows we really do attach the attributes to the substatement as expected:
```
// Test in test/AST/
if consteval [[likely]] {
}
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110482/new/

https://reviews.llvm.org/D110482