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

Ilya Biryukov via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jan 25 04:28:11 PST 2018


ilya-biryukov added inline comments.


================
Comment at: clangd/Function.h:141
 namespace detail {
+/// Optional-like type that sets its value to llvm::None when it was moved
+/// constructed from.
----------------
sammccall wrote:
> 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.
> 
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.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42525





More information about the cfe-commits mailing list