[PATCH] D50360: [Concepts] Requires Expressions

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/include/clang/Parse/Parser.h:804-807
+  bool TryAnnotateTypeOrScopeToken(bool ClassTemplateDeductionContext = true);
+  bool TryAnnotateTypeOrScopeTokenAfterScopeSpec(
+      CXXScopeSpec &SS, bool IsNewScope,
+      bool ClassTemplateDeductionContext = true);
----------------
Looks like the flag you're adding here is never actually used any more. Please remove it again.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:2296
+      S.flush();
+      if (Buf.find("typename ") != 0)
+        OS << "typename ";
----------------
rsmith wrote:
> I can see why this is the most straightforward way to implement this, but ... yuck. Please add a FIXME :)
I think you've actually fixed this now: we always build a `typename` type, so never need to print an extra `typename` here.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8291-8292
+
+  // Maintain an efficient lookup of params we have seen so far.
+  llvm::SmallSet<const IdentifierInfo*, 16> ParamsSoFar;
+
----------------
This variable is now unused; please remove it.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8309
+      CheckShadow(BodyScope, Param);
+      PushOnScopeChains(Param, BodyScope);
+    }
----------------
If we only do this now, do we properly handle `requires (int x, decltype(x) y)`? (Or is that handled in a separate function prototype scope which we've already left?)


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:10259-10261
+      else
+        Diag(CondRange.getBegin(), diag::err_typename_not_found_enable_if)
+            << CondRange;
----------------
Minor cleanup: I don't think we can ever reasonably get here for `enable_if<...>::type` with no `Ctx`. I mean, it's possible, but only from lexically within the definition of `enable_if` itself (or possibly a class derived from it?), and that's really not worth checking for. We could just skip calling `isEnableIf` and this whole `if` block if we don't have a `Ctx`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D50360