[Sema] Clean up ActionResult type a little. NFCI

[sammccall]

Summary

• Document the valid states: invalid/unset/pointer (Currently both documentation and implementation strongly suggest that pointer+invalid is poissible, when it's not)
• Remove unused set() functions, which had different semantics between the compressed/uncompressed specialization! (The former allowing escaping the tristate into pointer+invalid)
• Make the compressed specialization's internals directly model the tristate, rather than pretending pointer + invalid were independent.
• Make members of each version identical where possible, remove repetition
• fix operator= accidentally returning a const reference
• Fix indentation :-)

This was motivated by D157868, in which an experienced clang dev was
confused about the possible states for ExprResult - and I vividly
remember getting very confused about this myself.

[kadircet]

thanks a lot for doing this, i think this makes it a lot more obvious that this is a tristate struct rather than invalid just being a bit that can bit set for both null and non-null pointers.

clang/include/clang/Sema/Ownership.h

155

nit: also initialize Invalid to false?

204

nit: Value == InvalidValue ? nullptr : Value ?

[aaron.ballman]

Thank you for this! LGTM aside from a tiny nit.

clang/include/clang/Sema/Ownership.h

147

It might be worth tying this to usable.

[sammccall]

clang/include/clang/Sema/Ownership.h

204

I think that's probably slower, I don't see how the compiler could avoid actually emitting some sort of conditional.

Clang produces cmov: https://godbolt.org/z/croo6o3q6

I'm not 100% sure it matters, but it's a common path we expect to be trivial so i'd rather not change it here.

[tbaeder]

clang/include/clang/Sema/Ownership.h

180–183

clang/include/clang/Sema/Ownership.h

static inline clang::OpaquePtr<T> getFromVoidPointer(void *P) {
       return clang::OpaquePtr<T>::getFromOpaquePtr(P);
     }
   };
 
 } // namespace llvm
 
 namespace clang {
 
-  // Basic
 class StreamingDiagnostic;
 
 // Determines whether the low bit of the result pointer for the
 // given UID is always zero. If so, ActionResult will use that bit
 // for it's "invalid" flag.
 template <class Ptr> struct IsResultPtrLowBitFree {
   static const bool value = false;
-  };
 
-  /// ActionResult - This structure is used while parsing/acting on
-  /// expressions, stmts, etc.  It encapsulates both the object returned by
-  /// the action, plus a sense of whether or not it is valid.
-  /// When CompressInvalid is true, the "invalid" flag will be
-  /// stored in the low bit of the Val pointer.
-  template<class PtrTy,
-           bool CompressInvalid = IsResultPtrLowBitFree<PtrTy>::value>
-  class ActionResult {
-    PtrTy Val;
-    bool Invalid;
+/// The result of parsing/analyzing an expression, statement etc.
+///
+/// It may be:
+/// - a valid pointer to the result object
+/// - unset (null but valid), for constructs that may legitimately be absent
+///   (for example, the condition of a for loop)
+/// - invalid, indicating an error
+///   (no detail is provided, usually the error has already been diagnosed)
+template <class PtrTy, bool Compress = IsResultPtrLowBitFree<PtrTy>::value>
+  PtrTy Val = {};
 
-  public:
-    ActionResult(bool Invalid = false) : Val(PtrTy()), Invalid(Invalid) {}
-    ActionResult(PtrTy val) : Val(val), Invalid(false) {}
-    ActionResult(const DiagnosticBuilder &) : Val(PtrTy()), Invalid(true) {}
+  ActionResult(PtrTy Val) { *this = Val; }
+  ActionResult(const DiagnosticBuilder &) : ActionResult(/*Invalid=*/true) {}
 
-    // These two overloads prevent void* -> bool conversions.
-    ActionResult(const void *) = delete;
-    ActionResult(volatile void *) = delete;
 
-    bool isInvalid() const { return Invalid; }
-    bool isUsable() const { return !Invalid && Val; }
-    bool isUnset() const { return !Invalid && !Val; }
+  bool isUsable() const { return !isInvalid() && !isUnset(); }
 
-    PtrTy get() const { return Val; }
-    template <typename T> T *getAs() { return static_cast<T*>(get()); }
+  template <typename T> T *getAs() { return static_cast<T *>(get()); }
 
-    void set(PtrTy V) { Val = V; }
-
-    const ActionResult &operator=(PtrTy RHS) {
-      Val = RHS;
-      Invalid = false;
-      return *this;
-    }
-  };
+  ActionResult &operator=(PtrTy RHS) {
 
-  // This ActionResult partial specialization places the "invalid"
-  // flag into the low bit of the pointer.
-  template<typename PtrTy>
-  class ActionResult<PtrTy, true> {
-    // A pointer whose low bit is 1 if this result is invalid, 0
-    // otherwise.
-    uintptr_t PtrWithInvalid;
+// If we PtrTy has a free bit, we can represent "invalid" as nullptr|1.
+template <typename PtrTy> class ActionResult<PtrTy, true> {
+  static constexpr uintptr_t UnsetValue = 0x0;
+  static constexpr uintptr_t InvalidValue = 0x1;
+
+  uintptr_t Value = UnsetValue;
 
-    using PtrTraits = llvm::PointerLikeTypeTraits<PtrTy>;
 
-  public:
-    ActionResult(bool Invalid = false)
-        : PtrWithInvalid(static_cast<uintptr_t>(Invalid)) {}
-
-    ActionResult(PtrTy V) {
-      void *VP = PtrTraits::getAsVoidPointer(V);
-      PtrWithInvalid = reinterpret_cast<uintptr_t>(VP);
-      assert((PtrWithInvalid & 0x01) == 0 && "Badly aligned pointer");
-    }
-
-    ActionResult(const DiagnosticBuilder &) : PtrWithInvalid(0x01) {}
+      : Value(Invalid ? InvalidValue : UnsetValue) {}
+  ActionResult(PtrTy V) { *this = V; }
+  ActionResult(const DiagnosticBuilder &) : ActionResult(/*Invalid=*/true) {}
 
-    // These two overloads prevent void* -> bool conversions.
-    ActionResult(const void *) = delete;
-    ActionResult(volatile void *) = delete;
 
-    bool isInvalid() const { return PtrWithInvalid & 0x01; }
-    bool isUsable() const { return PtrWithInvalid > 0x01; }
-    bool isUnset() const { return PtrWithInvalid == 0; }
+  bool isInvalid() const { return Value == InvalidValue; }
+  bool isUnset() const { return Value == UnsetValue; }
+  bool isUsable() const { return !isInvalid() && !isUnset(); }
 
-    PtrTy get() const {
-      void *VP = reinterpret_cast<void *>(PtrWithInvalid & ~0x01);
-      return PtrTraits::getFromVoidPointer(VP);
-    }
-
-    template <typename T> T *getAs() { return static_cast<T*>(get()); }
+    void *VP = reinterpret_cast<void *>(Value & ~0x01);
+  template <typename T> T *getAs() { return static_cast<T *>(get()); }
 
-    void set(PtrTy V) {
-      void *VP = PtrTraits::getAsVoidPointer(V);
-      PtrWithInvalid = reinterpret_cast<uintptr_t>(VP);
-      assert((PtrWithInvalid & 0x01) == 0 && "Badly aligned pointer");
-    }
-
-    const ActionResult &operator=(PtrTy RHS) {
-      void *VP = PtrTraits::getAsVoidPointer(RHS);
-      PtrWithInvalid = reinterpret_cast<uintptr_t>(VP);
-      assert((PtrWithInvalid & 0x01) == 0 && "Badly aligned pointer");
-      return *this;
-    }
+  ActionResult &operator=(PtrTy RHS) {
+    Value = reinterpret_cast<uintptr_t>(VP);
+    assert((Value & 0x01) == 0 && "Badly aligned pointer");
 
-    // For types where we can fit a flag in with the pointer, provide
-    // conversions to/from pointer type.
-    static ActionResult getFromOpaquePointer(void *P) {
-      ActionResult Result;
-      Result.PtrWithInvalid = (uintptr_t)P;
-      return Result;
-    }
-    void *getAsOpaquePointer() const { return (void*)PtrWithInvalid; }
-  };
+    Result.Value = (uintptr_t)P;
+    assert(Result.isInvalid() ||
+           PtrTraits::getAsVoidPointer(Result.get()) == P);
+  void *getAsOpaquePointer() const { return (void *)Value; }
 
-  /// An opaque type for threading parsed type information through the
-  /// parser.
-  using ParsedType = OpaquePtr<QualType>;
-  using UnionParsedType = UnionOpaquePtr<QualType>;
+/// An opaque type for threading parsed type information through the parser.
 
-  // We can re-use the low bit of expression, statement, base, and
-  // member-initializer pointers for the "invalid" flag of
-  // ActionResult.
-  template<> struct IsResultPtrLowBitFree<Expr*> {
-    static const bool value = true;
-  };
-  template<> struct IsResultPtrLowBitFree<Stmt*> {
-    static const bool value = true;
-  };
-  template<> struct IsResultPtrLowBitFree<CXXBaseSpecifier*> {
-    static const bool value = true;
-  };
-  template<> struct IsResultPtrLowBitFree<CXXCtorInitializer*> {
-    static const bool value = true;
-  };
+template <> struct IsResultPtrLowBitFree<Expr *> {
+template <> struct IsResultPtrLowBitFree<Stmt *> {
+template <> struct IsResultPtrLowBitFree<CXXBaseSpecifier *> {
+template <> struct IsResultPtrLowBitFree<CXXCtorInitializer *> {
 
-  using ExprResult = ActionResult<Expr *>;
-  using StmtResult = ActionResult<Stmt *>;
-  using TypeResult = ActionResult<ParsedType>;
-  using BaseResult = ActionResult<CXXBaseSpecifier *>;
-  using MemInitResult = ActionResult<CXXCtorInitializer *>;
 
-  using DeclResult = ActionResult<Decl *>;
-  using ParsedTemplateTy = OpaquePtr<TemplateName>;
-  using UnionParsedTemplateTy = UnionOpaquePtr<TemplateName>;
 
-  using MultiExprArg = MutableArrayRef<Expr *>;
-  using MultiStmtArg = MutableArrayRef<Stmt *>;
-  using ASTTemplateArgsPtr = MutableArrayRef<ParsedTemplateArgument>;
-  using MultiTypeArg = MutableArrayRef<ParsedType>;
-  using MultiTemplateParamsArg = MutableArrayRef<TemplateParameterList *>;
 
-  inline ExprResult ExprError() { return ExprResult(true); }
-  inline StmtResult StmtError() { return StmtResult(true); }
-  inline TypeResult TypeError() { return TypeResult(true); }
 
-  inline ExprResult ExprError(const StreamingDiagnostic &) {
-    return ExprError();
-  }
-  inline StmtResult StmtError(const StreamingDiagnostic &) {
-    return StmtError();
-  }
+inline ExprResult ExprError(const StreamingDiagnostic &) { return ExprError(); }
+inline StmtResult StmtError(const StreamingDiagnostic &) { return StmtError(); }
 
-  inline ExprResult ExprEmpty() { return ExprResult(false); }
-  inline StmtResult StmtEmpty() { return StmtResult(false); }
 
-  inline Expr *AssertSuccess(ExprResult R) {
-    assert(!R.isInvalid() && "operation was asserted to never fail!");
-    return R.get();
-  }
 
-  inline Stmt *AssertSuccess(StmtResult R) {
-    assert(!R.isInvalid() && "operation was asserted to never fail!");
-    return R.get();
-  }
 
 } // namespace clang
 
 #endif // LLVM_CLANG_SEMA_OWNERSHIP_H