[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

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

rsmith added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:1983
+  /// \brief Types and expressions required to build C++2a three-way comparisons
+  ///   using operator<=>, including the values return by builtin <=> operators.
+  ComparisonCategories CompCategories;
----------------
We don't generally indent comments after a `\brief` like this. Also, we enable autobrief, so these `\brief`s are redundant.


================
Comment at: include/clang/AST/ComparisonCategories.h:56
+///   comparison. These values map onto instances of comparison category types
+///   defined in the standard library. i.e. 'std::strong_ordering::less'.
+enum class ComparisonCategoryResult : unsigned char {
----------------
You mean "eg", not "ie" here.


================
Comment at: include/clang/AST/ComparisonCategories.h:84-103
+    /// \brief True iff we've successfully evaluated the variable as a constant
+    /// expression and extracted its integer value.
+    bool hasValidIntValue() const { return HasValue; }
+
+    /// \brief Get the constant integer value used by this variable to represent
+    /// the comparison category result type.
+    llvm::APSInt getIntValue() const {
----------------
This seems unnecessary; we can get this information from the `VarDecl` instead. (You're caching a result here that is already cached there.)


================
Comment at: lib/AST/ComparisonCategories.cpp:25
+/// category result by evaluating the initializer for the specified VarDecl as
+/// a constant expression and retreiving the value of the classes first
+/// (and only) field.
----------------
classes -> class's


================
Comment at: lib/AST/ComparisonCategories.cpp:43-46
+  Expr::EvalResult Result;
+  if (!Info->VD->hasInit() ||
+      !Info->VD->getInit()->EvaluateAsRValue(Result, Ctx))
+    return true;
----------------
Use `VD->evaluateValue()` to get the cached value already stored by the `VarDecl`.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:959
+      !ArgTy->isMemberPointerType() && !ArgTy->isAnyComplexType()) {
+    return CGF.ErrorUnsupported(E, "aggregate three-way comparisoaoeun");
+  }
----------------
Typo. I'm almost tempted to say we should keep this for entertainment value, but on balance let's fix it :)


================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2899-2937
   assert(!location.getAs<NonLoc>() && "location cannot be a NonLoc.");
+
+  // Are we loading from a region?  This actually results in two loads; one
+  // to fetch the address of the referenced value and one to fetch the
+  // referenced value.
+  if (const auto *TR =
+        dyn_cast_or_null<TypedValueRegion>(location.getAsRegion())) {
----------------
This does not look related to your three-way comparison changes.


================
Comment at: test/SemaCXX/compare-cxx2a.cpp:40
 
-#if 0
+  (void)(A < 42);
   // (A,b)
----------------
Did you intend to add this here? It doesn't look related to the code under test.


================
Comment at: test/SemaCXX/std-compare-cxx2a.cpp:6
+void compare_not_found_test() {
+  // expected-error at +1 {{cannot deduce return type of 'operator<=>' because type partial_ordering was not found; include <compare>}}
+  (void)(0.0 <=> 42.123);
----------------
This diagnostic says just `partial_ordering` whereas the one below says `'std::partial_ordering'`. I prefer the latter more-explicit form, but in any case it would seem good to be consistent.


Repository:
  rL LLVM

https://reviews.llvm.org/D45476