[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:30
@@ +29,3 @@
+// An error with a string message describing the cause.
+class ErrorInfo : public llvm::ErrorInfo<ErrorInfo> {
+public:
----------------
It looks like most subclasses are named FooError, rather than FooErrorInfo.  Maybe SEError?

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:32
@@ +31,3 @@
+public:
+  ErrorInfo(std::string Message) : Message(Message) {}
+  void log(llvm::raw_ostream &OS) const override { OS << Message; }
----------------
Nit, if you take the string by value, probably better to std::move it.  But better still would be to take an llvm::StringRef, I think, and stringify it here.  Then we can create errors from Twines or whatever.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:38
@@ +37,3 @@
+  }
+  std::string GetMessage() const { return Message; }
+  static char ID;
----------------
Looking through existing code, this is usually called getErrorMessage.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:45
@@ +44,3 @@
+
+// Makes an error object based on a streamexecutor::ErrorInfo.
+//
----------------
nit, capital E

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:50
@@ +49,3 @@
+  return llvm::make_error<ErrorInfo>(std::forward<ArgTs>(Args)...);
+}
+
----------------
Given that there's only one constructor, I'm not sure we need a full-blown forwarding constructor.

But would it be so bad to just do "return make_error<SEError>("foo")" everywhere?  That is, do we need this wrapper?  "Explicit is better than implicit."

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:58
@@ +57,3 @@
+// If there is no error, the returned message matches the return value of
+// getSuccessMessage.
+static inline std::string consumeMessage(Error &&E) {
----------------
Should probably note that Error must be success or an SEError.

Kind of weird that we have no type-safety...

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:59
@@ +58,3 @@
+// getSuccessMessage.
+static inline std::string consumeMessage(Error &&E) {
+  if (!E) {
----------------
This seems like a strange name, because we're not really consuming the message.  consumeAndGetMessage?  Or is that too verbose?

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:79
@@ +78,3 @@
+  return consumeMessage(Exp.takeError());
+}
+
----------------
What if we didn't have this, and all callsites did

  consumeAndGetMessage(Exp.takeError());

AIUI you don't need the `if (Exp)` branch because takeError returns Error::success() if there's no error.

To me that would be more explicit, and it's not a ton more complicated.  But you have the context about the rest of the code. 

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:94
@@ +93,3 @@
+//
+// This is useful when the return type of the function is Expected<T>.
+#define SE_ERROR_RETURN_MOVED_IF_ERROR(x)                                      \
----------------
Hm, why would we sometimes want to std::move the error and sometimes not?  You don't want to std::move all the time because then we defeat rvo (and, I suppose, potentially get a warning about a pessimizing move)?

Do we really need these three macros?  On other projects I've worked on, they are widely viewed as a mistake, and in particular the distinction between the first two is mega-confusing.  And the precedent in llvm, such as it is, is to do without.  Maybe we can sit down and see whether the code becomes really ugly without them?

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:25
@@ +24,3 @@
+   static_cast<size_t>(!(sizeof(a) % sizeof(*(a)))))
+
+/// Marks a function return value so that the compiler emits a warning if that
----------------
Found it: STLExtras.h array_lengthof.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:40
@@ +39,3 @@
+#endif
+
+/// Checks that the input boolean expression is true
----------------
Maybe you forgot to update this file?

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:54
@@ +53,3 @@
+  } while (false)
+
+#endif // STREAMEXECUTOR_UTILS_MACROS_H
----------------
Same for this one.

================
Comment at: streamexecutor/include/streamexecutor/Utils/ThreadAnnotations.h:28
@@ +27,3 @@
+/// held when accessing the annotated variable.
+#define SE_GUARDED_BY(x) SE_THREAD_ANNOTATION_ATTRIBUTE__(guarded_by(x))
+
----------------
Would rather not pollute the global namespace with SE_THREAD_ANNOTATION_ATTRIBUTE if we can help it.  (That is, would rather declare this with one macro rather than two.)

But this also seems like something that would be better in e.g. mutex.h.


https://reviews.llvm.org/D22687