[PATCH] D31236: Refactor getHostCPUName to allow testing on non-native hardware.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added inline comments.


================
Comment at: include/llvm/Support/Host.h:83
+    virtual ssize_t readCpuInfo(void *Buf, size_t Size) const;
+    virtual StringRef getHostCPUName_powerpc() const;
+    virtual StringRef getHostCPUName_arm() const;
----------------
rengolin wrote:
> These don't need to be virtual, do they? They could even be static.
Ah right.
To be able to mock methods, they need to be virtual, so a Mock class can override them.
There are work-arounds to be able to mock non-virtual methods, but the work-arounds are worse than using virtual methods, IMHO.
See https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md, section "Mocking Nonvirtual Methods".

That being said, indeed, probably the getHostCPUName_xxx methods can be static non-virtual, as I don't see how these would need to be mocked.


================
Comment at: unittests/Support/Host.cpp:50
+  MOCK_CONST_METHOD2(readCpuInfo, ssize_t(void *Buf, size_t Size));
+  MOCK_CONST_METHOD0(getHostCPUName_powerpc, StringRef());
+};
----------------
No need to mock getHostCPUName_powerpc here.


================
Comment at: unittests/Support/Host.cpp:53
+
+TEST(getLinuxHostCPUName, ARM) {
+  const char *CortexA9ProcCpuinfo = R"(
----------------
rengolin wrote:
> Do we have tests for the other platforms?
Not at this point. I don't think this patch needs to introduce them, just make it easy for platform experts to add them in the test framework this patch introduces.


================
Comment at: unittests/Support/Host.cpp:90
+
+  EXPECT_THAT(m.getHostCPUName_arm(), Eq("cortex-a9"));
+}
----------------
rengolin wrote:
> There are a number of different ways to find cpu names on ARM cpuinfo, and it would be good to know that they're all working. Maybe having a few different small snippets, instead of one large and redundant one?
I imagine that over time, we'll add more tests and that most tests will uses a small snippet.
But, for example, in the future to be able to test big.LITTLE, probably it's best to use most of the /proc/cpuinfo content.
Also, right now, the implementation only actually reads the first 1024 bytes. When fixing that, we'll need a test with cpuinfo content larger than 1024 bytes.

In summary, in this patch I'm just aiming to demonstrate that it becomes possible to test this functionality also on other platforms, where before that wasn't possible. As a first test, I thought a simple full /proc/cpuinfo from a real system makes it slightly easier to read and understand rather than a further cut down input.


https://reviews.llvm.org/D31236