[libc-commits] [PATCH] D72743: [libc] Replace the use of gtest with a new light weight unittest framework.

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/src/sys/mman/munmap.h:12
 
-#include <sys/mman.h> // For size_t
+#include "include/sys/mman.h" // For size_t
 
----------------
While we are here, the other comments end with a period so this one should too.


================
Comment at: libc/test/src/string/strcat_test.cpp:19
+  char *result = __llvm_libc::strcat(dest, abc);
+  ASSERT_STREQ(dest, result);
+  ASSERT_STREQ(dest, abc);
----------------
Shouldn't we keep this as just `ASSERT_EQ`, the streq doesn't make much sense because these should be the same pointers. I think right now ASSERT_EQ with char* will not work but see my other comments.


================
Comment at: libc/utils/UnitTest/Test.cpp:42
+                 const char *File, unsigned long Line) {
+  if (Cond == llvm_libc::testing::Test::Cond_EQ) {
+    if (LHS == RHS)
----------------
Maybe a `using llvm_libc::testing::Test;` and shorten these to `Test::Cond_EQ`?


================
Comment at: libc/utils/UnitTest/Test.cpp:47
+    Ctx.markFail();
+    llvm::outs() << File << ":" << Line << ": FAILURE\n"
+                 << "      Expected: " << LHSStr << '\n'
----------------
What about `llvm::withColor(llvm::errs(), llvm::raw_ostream::RED);` Even if you don't like the color, all of these should be printing to stderr.


================
Comment at: libc/utils/UnitTest/Test.cpp:132
+
+void Test::addTest(Test *T) {
+  if (End == nullptr) {
----------------
What if `addTest` added to a vector that way we can easily change the order the tests are executed for like in `--gtest_shuffle`


================
Comment at: libc/utils/UnitTest/Test.cpp:150
+  int FailCount = 0;
+  do {
+    ++TestCount;
----------------
`for (Test *T = Start; T; T = T->Next, ++TestCount)`

And then the
```
 Test *T = Start;
  if (T == nullptr)
    return 0;
```
can be removed along with the `++TestCount` right below this comment.


================
Comment at: libc/utils/UnitTest/Test.cpp:174
+
+  return FailCount > 0 ? 1 : 0;
+}
----------------
What about `!!FailCount`?


================
Comment at: libc/utils/UnitTest/Test.cpp:177-183
+template <>
+bool Test::test<char>(RunContext &Ctx, Test::Condition Cond, char LHS, char RHS,
+                      const char *LHSStr, const char *RHSStr, const char *File,
+                      unsigned long Line) {
+  return ::test(Ctx, Cond, LHS, RHS, LHSStr, RHSStr, File, Line);
+}
+
----------------
You can create

```
template <typename T>
bool Test::test(...) {
   return ::test(...);
}
```

and then instantiate each of these

`template Test::test<int>(RunContext &, ...);`

But see previous comment I think we can do this differently.


================
Comment at: libc/utils/UnitTest/Test.h:55-58
+  template <typename ValType>
+  static bool test(RunContext &Ctx, Condition Cond, ValType LHS, ValType RHS,
+                   const char *LHSStr, const char *RHSStr, const char *File,
+                   unsigned long Line);
----------------
We can make this template call ::test(...) just here. But also It would be worth it to have enable_if specialization for std::is_pointer to call ::test<const void*>, and std::is_integral to call ::test<int64_t> and one for unsigned integrals to call use uint64_t so that we only need to have a couple instantiated in Test.cpp.


================
Comment at: libc/utils/UnitTest/Test.h:102-105
+#define ASSERT_EQ(LHS, RHS)                                                    \
+  if (!llvm_libc::testing::Test::test(Ctx, Cond_EQ, (LHS), (RHS), #LHS, #RHS,  \
+                                      __FILE__, __LINE__))                     \
+  return
----------------
What do you think about making the ASSERT_*'s into `if (!EXPECT_*) return`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72743