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

[Chandler Carruth via Parallel_libs-commits]

chandlerc added a subscriber: chandlerc.
chandlerc added a comment.

(somewhat of a drive-by comment below, sorry for that, but just wanted to try and push comments and documentation in a small way)


================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:25-27
@@ +24,5 @@
+
+using llvm::Error;
+using llvm::Expected;
+using llvm::consumeError;
+
----------------
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.


https://reviews.llvm.org/D22687