This is an archive of the discontinued LLVM Phabricator instance.

[libc] change str to int tests to be templated
ClosedPublic

Authored by michaelrj on Dec 20 2022, 3:43 PM.

Details

Summary

Previously the tests were copy/pasted into several files, this changes
them to be instead templated and sharing one file.

Diff Detail

Event Timeline

michaelrj created this revision.Dec 20 2022, 3:43 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 20 2022, 3:43 PM
michaelrj requested review of this revision.Dec 20 2022, 3:43 PM
lntue added a comment.Dec 20 2022, 7:41 PM

Look like you'll need to update the bazel layout for the tests. It complains about missing files.

libc/test/src/stdlib/StrtolTest.h
165–169

This pattern is repeated a lot in this test. Maybe you can factor it to another method or macro then?

michaelrj updated this revision to Diff 484697.Dec 21 2022, 3:12 PM

address comment and update bazel

libc/test/src/stdlib/StrtolTest.h
165–169

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.

lntue added inline comments.Dec 21 2022, 6:10 PM
libc/test/src/stdlib/StrtolTest.h
165–169

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.

michaelrj marked 2 inline comments as done.

add a comment and rebase

libc/test/src/stdlib/StrtolTest.h
165–169

I've added a comment at the top to look into it.

lntue accepted this revision.Dec 22 2022, 10:29 AM
This revision is now accepted and ready to land.Dec 22 2022, 10:29 AM
This revision was automatically updated to reflect the committed changes.