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

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:25-27
@@ +24,5 @@
+
+using llvm::Error;
+using llvm::Expected;
+using llvm::consumeError;
+
----------------
chandlerc wrote:
> If I were a naive reader, I might not understand the context that leads you to specifically pull these types into the streamexecutor namespace.
> 
> I think it would be really useful to expand the comments in the code to include more rationale behind the design the code is following. As an example, the file comment above might talk about the fact that these aren't just internal utilities of streamexecutor, but types that will in some cases appear on the user-facing side of the streamexecutor APIs, and thus form some of the building blocks for streamexecutor's user-facing API. As a consequence, while they are implemented in terms of the LLVM primitives, they are specialized and provided under that namespace to make things more clear for users, blah blalh blah.
> 
> Anyways, a very meta comment, and not even necessarily something you want to address in this patch vs. other patches, and probably this is just one place where it might be relevant.
Great point. I've added a ton of comments now.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:50
@@ +49,3 @@
+  return llvm::make_error<ErrorInfo>(std::forward<ArgTs>(Args)...);
+}
+
----------------
jlebar wrote:
> 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."
I've refactored so that "SEError" is not present in the header. This makes my original intention clear, that the user should not know about StreamExecutor's custom llvm::ErrorInfo class. I've retained the make_error function, but now it's not templated, but instead just takes a StringRef.

================
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) {
----------------
jlebar wrote:
> Should probably note that Error must be success or an SEError.
> 
> Kind of weird that we have no type-safety...
I've added a few comments that talk about how only the public functions should be used to deal with these types. It is weird that there's no type safety, but if people stick to the public interface in namespace streamexecutor, I don't think they can go wrong.

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

================
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)                                      \
----------------
jlebar wrote:
> 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?
We don't really need these macros. They just came from emulating Google internal StreamExecutor. I have removed them now.

I'll say why I had move and non-move versions here, but feel free to skip as they are gone now. The move version was for functions that returned Expected<T>. Expected<T> has a constructor that takes an rvalue ref to Error, so we would have to return a moved temporary error in order to be able to call that constructor.

================
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
----------------
jlebar wrote:
> Found it: STLExtras.h array_lengthof.
Awesome! Thanks. Now I can get rid of this whole macros file.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:40
@@ +39,3 @@
+#endif
+
+/// Checks that the input boolean expression is true
----------------
jlebar wrote:
> Maybe you forgot to update this file?
Yep, I just forgot to update it. The whole file should be gone now.

================
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))
+
----------------
jlebar wrote:
> 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.
Consolidated to one macro and moved to Mutex.h.


https://reviews.llvm.org/D22687