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

Renato Golin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 22 05:32:36 PDT 2017


rengolin 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;
----------------
kristof.beyls wrote:
> 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.
Makes sense...


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


================
Comment at: unittests/Support/Host.cpp:53
+
+TEST(getLinuxHostCPUName, ARM) {
+  const char *CortexA9ProcCpuinfo = R"(
----------------
kristof.beyls wrote:
> 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.
Ok.


================
Comment at: unittests/Support/Host.cpp:90
+
+  EXPECT_THAT(m.getHostCPUName_arm(), Eq("cortex-a9"));
+}
----------------
kristof.beyls wrote:
> 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.
Sure, but in this case you just need to look at 0x09. :)

I mean, at this moment, it would be more value to have a few strings and testing for different CPUs than the whole string testing for a single CPU.


https://reviews.llvm.org/D31236





More information about the llvm-commits mailing list