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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
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);
----------------
abrachet wrote:
> 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.
I have added a new `ASSERT_PTREQ` macro to test pointer equality. I previously had a specialization of `ASSERT_EQ` for `void *`. But, I think something like a `PTREQ` it is more useful as it can be used to compare any kind of pointer.


================
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)
----------------
abrachet wrote:
> Maybe a `using llvm_libc::testing::Test;` and shorten these to `Test::Cond_EQ`?
Its affecting only this function so I have left it as is.


================
Comment at: libc/utils/UnitTest/Test.cpp:47
+    Ctx.markFail();
+    llvm::outs() << File << ":" << Line << ": FAILURE\n"
+                 << "      Expected: " << LHSStr << '\n'
----------------
abrachet wrote:
> 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.
Streaming to `llvm::errs()` instead of `llvm::outs()`.


================
Comment at: libc/utils/UnitTest/Test.cpp:132
+
+void Test::addTest(Test *T) {
+  if (End == nullptr) {
----------------
abrachet wrote:
> What if `addTest` added to a vector that way we can easily change the order the tests are executed for like in `--gtest_shuffle`
Yeah, I did consider that. But, that means we will have a global non-POD which is usually not considered as a good practice. We can always reconsider this in case we want features like `--gtest-shuffle`.


================
Comment at: libc/utils/UnitTest/Test.cpp:174
+
+  return FailCount > 0 ? 1 : 0;
+}
----------------
abrachet wrote:
> What about `!!FailCount`?
Personally, I do not like the implicit nature of such a construct.


================
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);
+}
+
----------------
abrachet wrote:
> 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.
I do not think a declaration of a function specialization instantiates it. Am I misunderstanding something?


================
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);
----------------
abrachet wrote:
> 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.
This a very good suggestion to `enable_if`. But, I do not want to include `<type_traits>` here so I have now implemented an equivalent and used it to incorporate your suggestion. The benefit of using it is that the use of unsupported types leads to a compile-error which is more descriptive than a link-error (which was the case earlier.)

About calling `::test`, since `::test` is template function, we will have to define it in the header. We will then have to include some headers which we want to avoid.
About filtering signed integers to `int64_t` comparison etc., I want to avoid such implicit conversions so I have kept it as is.
About using `std::is_pointer`, I have added new `EXPECT_PTREQ` and friends. These macros cast the pointers to `unsigned long long` and do the comparison. 


================
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
----------------
abrachet wrote:
> What do you think about making the ASSERT_*'s into `if (!EXPECT_*) return`?
This is yet another good suggestion. Done now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72743