[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/Demangle.h:23
@@ +22,3 @@
+/// Demangle a mangled name.
+std::string demangle(const char *Mangled);
+
----------------
I would be pretty surprised if we didn't already have a function for this in llvm.  I think you can call LLVMSymbolizer::DemangleName with a null ModInfo?

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:1
@@ +1,2 @@
+//===-- Error.h - Error codes -----------------------------------*- C++ -*-===//
+//
----------------
We already have support/Error.h, which I think is our idiom, subsuming this, Status, and StatusOr.  This is the first time I've looked at these facilities, so I don't know e.g. whether we want to use ErrorOr<> or Expected<>.

If we end up keeping this, it seems like we probably don't need many of these error codes?  But we can cross that bridge.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:24
@@ +23,3 @@
+  ((sizeof(a) / sizeof(*(a))) /                                                \
+   static_cast<size_t>(!(sizeof(a) % sizeof(*(a)))))
+
----------------
Surely we have one of these in LLVM.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:39
@@ +38,3 @@
+#define SE_WARN_UNUSED_RESULT
+#endif
+
----------------
LLVM_ATTRIBUTE_UNUSED_RESULT

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:53
@@ +52,3 @@
+    static_cast<void>(ok);                                                     \
+  } while (false)
+
----------------
Hm.  This is un-llvm-ish.  I'd rather not introduce this idiom into LLVM, since nobody else is using it.

Is this used all over the place?

================
Comment at: streamexecutor/include/streamexecutor/Utils/Mutex.h:50
@@ +49,2 @@
+
+#endif // STREAMEXECUTOR_UTILS_MUTEX_H
----------------
LLVM has a Mutex class.

================
Comment at: streamexecutor/include/streamexecutor/Utils/PtrUtil.h:60
@@ +59,2 @@
+
+#endif // STREAMEXECUTOR_UTILS_PTRUTIL_H
----------------
This already exists in STLExtras.h.

================
Comment at: streamexecutor/lib/Utils/StrUtil.cpp:1
@@ +1,2 @@
+//===-- StrUtil.cc - String utility implementations -----------------------===//
+//
----------------
I am not sure we need these utils.

StringRef has lower(), upper(), startswith(), and endswith(), and StringExtras.h has SplitString.  The only one I'm not sure about is stripSuffixString.  That one seems less general-purpose, but if we use it a lot, maybe we can move it to StringExtras.h?


https://reviews.llvm.org/D22687