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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin 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();
+    }
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110482