[Parallel_libs-commits] [PATCH] D24148: [StreamExecutor] Read dev array directly in test

[Jason Henline via Parallel_libs-commits]

jhen marked an inline comment as done.

================
Comment at: streamexecutor/lib/unittests/SimpleHostPlatformDevice.h:30
@@ -29,3 +29,3 @@
 /// std::memcpy.
 class SimpleHostPlatformDevice : public streamexecutor::PlatformDevice {
   std::string getName() const override { return "SimpleHostPlatformDevice"; }
----------------
jlebar wrote:
> Hm, I just noticed this whole file is in the global namespace.  Is that intentional?
I thought that was the right thing to do for test code, but looking at LLVM and clang shows that's not how it's done there. They put header test code in namespace llvm and clang, respectively. Still, I don't want this code on an equal footing with the library code, so I put it in namespace streamexecutor::test. Does that seem reasonable?

================
Comment at: streamexecutor/lib/unittests/SimpleHostPlatformDevice.h:138
@@ +137,3 @@
+template <typename T>
+T getDeviceValue(const streamexecutor::GlobalDeviceMemory<T> &Memory,
+                 size_t Index) {
----------------
jlebar wrote:
> This seems like an awfully generic name...  I guess it's only for unit tests, but still...
> 
> It could be a static function on SimpleHostPlatformDevice?
You're right, I think it works better as a static function in SimpleHostPlatformDevice. I moved it there.


https://reviews.llvm.org/D24148