[Parallel_libs-commits] [PATCH] D22687: [StreamExecutor] Add utility library

[Justin Lebar via Parallel_libs-commits]

jlebar added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:27
@@ +26,3 @@
+/// before they go out of scope, otherwise they will cause the program to abort
+/// with a warning about an unchecked error. If the error represents success,
+/// then checking the boolean value is all that is required, but if the error
----------------
Probably worth capitalizing "Error" when we're talking about the class.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:31
@@ +30,3 @@
+/// consumeAndGetMessage is the way to extract the error message from an error
+/// message and consume the error at the same time, so typical error handling
+/// will first check whether there was an error and then extract the error
----------------
from an Error object

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:78
@@ +77,3 @@
+/// they contain a value and false if they contain an error. Note that this is
+/// the opposite convention of the Error type conversion to bool where true
+/// meant error and false meant success.
----------------
Nit, ", where"

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:82
@@ +81,3 @@
+/// If the Expected<T> instance is not an error, the stored value can be
+/// obtained by using the * pointer dereference operator. If access to members
+/// of the value are desired instead of the value itself, the -> pointer
----------------
suggest s/the * pointer dereference operator/operator*/.  Similarly below for ->

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:121
@@ +120,3 @@
+/// Or if the containing function returns an Expected<U>, the error can be
+/// passed up the stack:
+///
----------------
Maybe we should say

"You can also write the same code if doTask3() returns an Expected<U> instead of an Error."  One less example, and it calls out the equivalence of the code rather than asking the reader to figure out that it's the same.

On another note, could I do

  return ExpectedInt;

instead of 

  return ExpectedInt.takeError();

I think so based on the rules above.  Not sure if that's worth mentioning, or if we have a preference between these two ways of doing it.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:180
@@ +179,3 @@
+// error.
+static inline std::string getSuccessMessage() { return "success"; }
+
----------------
I wonder if we need this one.  It may encourage people to do string comparisons to check whether a function returned success, and that's kind of lame.

What do you think, based on the code you haven't released yet?

================
Comment at: streamexecutor/lib/Utils/Error.cpp:17
@@ +16,3 @@
+
+#include <llvm/ADT/StringRef.h>
+
----------------
Why <> instead of ""?  Same in the header.  llvm and clang don't contain the string "#include <llvm".

================
Comment at: streamexecutor/lib/Utils/Error.cpp:25
@@ +24,3 @@
+  StreamExecutorError(llvm::StringRef Message)
+      : Message(Message.begin(), Message.end()) {}
+
----------------
Nit, suggest `Message.str()` -- a bit simpler.


https://reviews.llvm.org/D22687