[clangd] Replace Optional in ScopeExitGuard (fix after r322838)

[ilya-biryukov]

Summary

r322838 changed semantics for llvm::Optional that removes the
guarantee of setting the value to empty after move.
This change creates a wrapper around Optional that gives this
guarantee, it is useful for code dealing with empty values that could
be moved.

[sammccall]

clangd/Function.h

141

As noted in the thread I don't think this pulls its weight for us.
Swapping unique_ptr for optional doesn't seem like it'll ever be a bottleneck for us. If we really must avoid the allocation for some reason then adding move logic to ScopeExitGuard seems simpler.
If you feel strongly about keeping this as a separate concept, it should really go in llvm/ADT/Optional.h, and probably needs a new name.

[ilya-biryukov]

clangd/Function.h

141

It's not a bottleneck, but I don't see why something like ScopeExitGuard should incur an overhead of an extra alloc that needs to be done by unique_ptr.
That also seems like a nice reusable concept for move-only things.

I don't subscribe to the view that this class carries much weight. It's a really simple wrapper. It will probably get much more complicated if we want to support the general case (i.e. make it a substitute of llvm::Optional with extra semantics). And I don't see how we can put it into ADT/Optional.h without supporting the general case.

[sammccall]

r323443 removed ScopeExitGuard altogether.

clangd/Function.h

 /// std::forwarded'ed (therefore can be moved) into \p F during the call.
 template <class Func, class... Args>
 ForwardBinder<Func, Args...> BindWithForward(Func F, Args &&... As) {
   return ForwardBinder<Func, Args...>(
       std::make_tuple(std::forward<Func>(F), std::forward<Args>(As)...));
 }
 
 namespace detail {
+/// Optional-like type that sets its value to llvm::None when it was moved
+/// constructed from.
+template <class T> class CleaningOptional {
+public:
+  static_assert(std::is_same<typename std::decay<T>::type, T>::value,
+                "Implementation currently only supports decayed types");
+
+  CleaningOptional(T Value) : Value(std::move(Value)) {}
+
+  CleaningOptional(CleaningOptional &&Other) : Value(std::move(Other.Value)) {
+    Other.Value = llvm::None;
+  }
+
+  CleaningOptional &operator=(CleaningOptional &&Other) {
+    Value = std::move(Other.Value);
+    Other.Value = llvm::None;
+  }
+
+  explicit operator bool() const { return static_cast<bool>(Value); }
+
+  T &operator*() { return *Value; }
+  const T &operator*() const { return *Value; }
+
+private:
+  llvm::Optional<T> Value;
+};
+
 /// Runs provided callback in destructor. Use onScopeExit helper function to
 /// create this object.
 template <class Func> struct ScopeExitGuard {
   static_assert(std::is_same<typename std::decay<Func>::type, Func>::value,
                 "Func must be decayed");
 
   ScopeExitGuard(Func F) : F(std::move(F)) {}
   ~ScopeExitGuard() {
     if (!F)
       return;
     (*F)();
   }
 
   // Move-only.
   ScopeExitGuard(const ScopeExitGuard &) = delete;
   ScopeExitGuard &operator=(const ScopeExitGuard &) = delete;
 
   ScopeExitGuard(ScopeExitGuard &&Other) = default;
   ScopeExitGuard &operator=(ScopeExitGuard &&Other) = default;
 
 private:
-  llvm::Optional<Func> F;
+  CleaningOptional<Func> F;
 };
 } // namespace detail
 
 /// Creates a RAII object that will run \p F in its destructor.
 template <class Func>
 auto onScopeExit(Func &&F)
     -> detail::ScopeExitGuard<typename std::decay<Func>::type> {
   return detail::ScopeExitGuard<typename std::decay<Func>::type>(
       std::forward<Func>(F));
 }
 
 } // namespace clangd
 } // namespace clang
 
 #endif