[libc-commits] [PATCH] D140441: [libc] change str to int tests to be templated

[Michael Jones via Phabricator via libc-commits]

michaelrj added inline comments.


================
Comment at: libc/test/src/stdlib/StrtolTest.h:164-168
+    const char *sign_after = "2+2=4";
+    errno = 0;
+    ASSERT_EQ(func(sign_after, &str_end, 10), ReturnT(2));
+    ASSERT_EQ(errno, 0);
+    EXPECT_EQ(str_end - sign_after, ptrdiff_t(1));
----------------
lntue wrote:
> michaelrj wrote:
> > lntue wrote:
> > > This pattern is repeated a lot in this test.  Maybe you can factor it to another method or macro then?
> > I think for clarity having this pattern repeated is okay. There are three separate asserts and being able to see them individually instead of needing to translate a macro is better when debugging.
> > 
> > While I could change these to be
> > ```
> > #define STRTOL_TEST(str, base, return_val, expected_errno, expected_strlen) \
> > errno = 0; \
> > ASSERT_EQ(func(str, &str_end, base), ReturnT(return_val)); \
> > ASSERT_EQ(errno, expected_errno); \
> > EXPECT_EQ(str_end - str, ptrdiff_t(expected_strlen));
> > 
> > STRTOL_TEST(signafter, 10, 2, 0, 1)
> > ```
> > 
> > I think that last line so short as to be unreadable. If there was a test failure I'd have to go back and read through the macro every time to remember which number is which. 
> > While I could make a simpler macro that takes just the input string and the output number and just assumes the other values (errno = 0, base = 10, string length = sizeof(string)), that wouldn't work for some of these tests (such as this one). That means we'd probably have to leave those tests as they are, and given that they make up a significant portion of the tests, I think it's better to just leave them all as they are.
> I think you could add the following method to `StrtoTest` class:
> ```
> void Check(FunctionT func, const char *str, int base ReturnT return_val, int expected_errno, int expected_strlen) {
>   char *str_end = nullptr;
>   errno = 0;
>   ASSERT_EQ(func(str, &str_end, base), return_val);
>   ASSERT_EQ(errno, expected_errno);
>   EXPECT_EQ(str_end - str, ptrdiff_t(expected_strlen));
> }
> ```
> and then the stack trace should show you the exact failed statement, especially if the call sites are notated with parameter names:
> ```
>   Check("2+2=4", /*base*/ 10, /*return_val*/ ReturnT(2), /*expected_errno*/ 0, /*expected_strlen*/ 1);
> ```
> 
> This is a lot of changes for the current tests, so maybe you can create a clean up task/issue/TODO to be done later.
I've added a comment at the top to look into it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140441/new/

https://reviews.llvm.org/D140441