[PATCH] D17778: TypedError for recoverable error handling

[Lang Hames via llvm-commits]

lhames added inline comments.

================
Comment at: docs/ProgrammersManual.rst:373
@@ +372,3 @@
+    if (f < 0)
+      return make_typed_error<FloatingPointError>(...);
+    return sqrt(f);
----------------
joker.eph wrote:
> `make_typed_error` is a bit "long" (the "..." here will almost always lead to wrapping (bad for vertical space efficiency).
> What about:
> 
> `return TypedError<FloatingPointError>(...)`
> 
> The same remark applies to all the API that repeat "TypedError" everywhere (`catchTypedError`, etc.). Can we at least drop the "typed" part where possible?
> 
So far I've heard general enthusiasm (and no objections) to renaming this class and the utilities to just "Error". I'll re-work the patch - that should cut down on this boilerplate a little.

================
Comment at: include/llvm/Support/TypedError.h:148
@@ +147,3 @@
+    Other.setPtr(nullptr);
+    Other.setChecked(true);
+  }
----------------
joker.eph wrote:
> Could this ctor be expressed in terms of the move assignment operator?
Yes it could. I'll fix that up.

================
Comment at: include/llvm/Support/TypedError.h:355
@@ +354,3 @@
+///
+/// class MyError : pubilc TypedErrorInfo<MyError> {};
+///
----------------
joker.eph wrote:
> s/pubilc/public
Good catch.

================
Comment at: include/llvm/Support/TypedError.h:359
@@ +358,3 @@
+/// checkAllErrors(std::move(E),
+///   handle<MyError>([](std::unique_ptr<MyError> M) {
+///     ...
----------------
joker.eph wrote:
> I like reading the `handle<MyError>(...)` but won't it be actually `handle<MyError>(...)`?
I don't follow the question?

If you mean that this syntax is a bit verbose - I agree. I'll see if I can improve this.

================
Comment at: include/llvm/Support/TypedError.h:499
@@ +498,3 @@
+/// type.
+template <class T> class TypedErrorOr {
+  template <class OtherT> friend class TypedErrorOr;
----------------
joker.eph wrote:
> I assumed you almost copy/pasted the `ErrorOr` implementation right? Some weirdness here and there but it has been pretty well tested so...
I did. :)

================
Comment at: include/llvm/Support/TypedError.h:537
@@ +536,3 @@
+
+  // This might eventually need SFINAE but it's more complex than is_convertible
+  // & I'm too lazy to write it right now.
----------------
joker.eph wrote:
> What do you mean?
This comment was copied over from ErrorOr - I think it's stale. I'll get rid of it.

================
Comment at: include/llvm/Support/TypedError.h:554
@@ +553,3 @@
+    if (!HasError)
+      getStorage()->~storage_type();
+  }
----------------
joker.eph wrote:
> The error storage never needs deletion?
> Is it handled differently somehow?
> 
Oops. This is an omission. ErrorOr doesn't need to delete its error storage because it's just a std::error_code, but TypedErrorOr does. I'll fix that up.


Repository:
  rL LLVM

http://reviews.llvm.org/D17778