[PATCH] D17778: TypedError for recoverable error handling

[Mehdi AMINI via llvm-commits]

joker.eph added a comment.

That's a great patch overall! I enjoyed reading it :)

Please see comments below.


================
Comment at: docs/ProgrammersManual.rst:304
@@ +303,3 @@
+    default:
+      llvm_unreachable("Illegal value for X");
+  }
----------------
Don't you get warning for default with fully-covered switch in this case?

================
Comment at: docs/ProgrammersManual.rst:373
@@ +372,3 @@
+    if (f < 0)
+      return make_typed_error<FloatingPointError>(...);
+    return sqrt(f);
----------------
`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?


================
Comment at: include/llvm/Support/TypedError.h:148
@@ +147,3 @@
+    Other.setPtr(nullptr);
+    Other.setChecked(true);
+  }
----------------
Could this ctor be expressed in terms of the move assignment operator?

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

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

================
Comment at: include/llvm/Support/TypedError.h:496
@@ +495,3 @@
+/// This class parallels ErrorOr, but replaces error_code with TypedError. Since
+/// TypedError cannot be copied, this clas replaces getError() with takeError().
+/// It also adds an bool errorIsA<ErrT>() method for testing the error class
----------------
s/cla/class/

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

================
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.
----------------
What do you mean?

================
Comment at: include/llvm/Support/TypedError.h:554
@@ +553,3 @@
+    if (!HasError)
+      getStorage()->~storage_type();
+  }
----------------
The error storage never needs deletion?
Is it handled differently somehow?


================
Comment at: include/llvm/Support/TypedError.h:642
@@ +641,3 @@
+
+template <class T, class E>
+typename std::enable_if<std::is_error_code_enum<E>::value ||
----------------
Doc.


Repository:
  rL LLVM

http://reviews.llvm.org/D17778