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

[Jason Henline via Parallel_libs-commits]

jhen added a comment.

I followed most of jlebar's suggestions to use standard LLVM utilities and this patch is now much smaller.


================
Comment at: streamexecutor/include/streamexecutor/Utils/Demangle.h:23
@@ +22,3 @@
+/// Demangle a mangled name.
+std::string demangle(const char *Mangled);
+
----------------
jlebar wrote:
> 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?
You're right, LLVMSymbolizer::DemangleName seems to work. So, I got rid of all the SE custom "Demangle" stuff.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Error.h:1
@@ +1,2 @@
+//===-- Error.h - Error codes -----------------------------------*- C++ -*-===//
+//
----------------
jlebar wrote:
> 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.
I did a lot of experiments and Expected<> looks like it does what we want, so I removed Status and StatusOr and replaced them with Error.h which wraps llvm::Error and llvm::Expected.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:24
@@ +23,3 @@
+  ((sizeof(a) / sizeof(*(a))) /                                                \
+   static_cast<size_t>(!(sizeof(a) % sizeof(*(a)))))
+
----------------
jlebar wrote:
> Surely we have one of these in LLVM.
I searched but couldn't find one. I even grepped in `include/llvm` using `grep -R '\<sizeof(.*)[[:space:]]*/[[:space:]]*sizeof('` and it only showed up some unrelated stuff in Support/MachO.h.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:39
@@ +38,3 @@
+#define SE_WARN_UNUSED_RESULT
+#endif
+
----------------
jlebar wrote:
> LLVM_ATTRIBUTE_UNUSED_RESULT
Now that Status has been deleted, this is not needed anymore anyway, so I got rid of it.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Macros.h:53
@@ +52,3 @@
+    static_cast<void>(ok);                                                     \
+  } while (false)
+
----------------
jlebar wrote:
> 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?
This isn't widely used and it's easy to replace. It's gone now.

================
Comment at: streamexecutor/include/streamexecutor/Utils/Mutex.h:50
@@ +49,2 @@
+
+#endif // STREAMEXECUTOR_UTILS_MUTEX_H
----------------
jlebar wrote:
> LLVM has a Mutex class.
Cool, I got rid of this file and I'll switch to using LLVM's RWMutex when I want a shared_mutex.

================
Comment at: streamexecutor/include/streamexecutor/Utils/PtrUtil.h:60
@@ +59,2 @@
+
+#endif // STREAMEXECUTOR_UTILS_PTRUTIL_H
----------------
jlebar wrote:
> This already exists in STLExtras.h.
Great! I switched to using the STLExtras version and removed this.

================
Comment at: streamexecutor/lib/Utils/StrUtil.cpp:1
@@ +1,2 @@
+//===-- StrUtil.cc - String utility implementations -----------------------===//
+//
----------------
jlebar wrote:
> 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?
You're right, they're not needed. I switch to using the StringRef stuff now and removed StrUtil.


https://reviews.llvm.org/D22687