[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/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);
+}
+
----------------
sivachandra wrote:
> 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?
It instantiates if you write like `template function<params_to_instantiate>(...);`. Its used more often with class templates but it works with function templates as well. In llvm, I have seen it [[ https://github.com/llvm/llvm-project/blob/master/llvm/tools/llvm-objcopy/ELF/Object.cpp#L2388 | here ]].


================
Comment at: libc/utils/UnitTest/Test.cpp:14
+
+#include <string>
+
----------------
This include is unnecessary I think.


================
Comment at: libc/utils/UnitTest/Test.cpp:148
+    const char *TestName = T->getName();
+    llvm::outs() << "[ RUN      ] " << TestName << '\n';
+    RunContext Ctx;
----------------
This one is still to `outs`


================
Comment at: libc/utils/UnitTest/Test.cpp:155
+    if (Result == RunContext::Result_Fail) {
+      llvm::outs() << "[  FAILED  ] " << TestName << '\n';
+      ++FailCount;
----------------
ditto


================
Comment at: libc/utils/UnitTest/Test.cpp:158
+    } else if (Result == RunContext::Result_Pass) {
+      llvm::outs() << "[       OK ] " << TestName << '\n';
+    } else {
----------------
ditto


================
Comment at: libc/utils/UnitTest/Test.cpp:164
+
+  llvm::outs() << "Ran " << TestCount << " tests. "
+               << " PASS: " << TestCount - FailCount << ' '
----------------
ditto


================
Comment at: libc/utils/UnitTest/Test.h:23
+
+template <typename Type> struct IsIntegerType {
+  static const bool Value = false;
----------------
It looks like [[ https://github.com/llvm/llvm-project/blob/master/libcxx/include/type_traits#L736 | libcxx ]] does remove_cv before these specializations. Which confuses me because
``` 
template <typename T> void f(T) {}
template <> void f<char>(char) {}
int main() {
  const volatile char c = 'c';
  f(c); // calls f<char>
}
```
But I figured I would mention it.


================
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);
----------------
sivachandra wrote:
> 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. 
>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.

Thats no different than what we have now we have the declaration of `Test::test` and it's definition in the cpp file. Just putting `template <typename ValType> bool ::test(...);` in the header file and then making `Test::test` call `::test` makes it inline-able but more importantly its a little easier to follow I think.

> 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.

I think it is important that we make a `IsPointerType` specialization so we can just use `EXPECT_EQ` and be consistent with gtest. This will confuse people later I'm pretty sure. Also if we do the above suggestion this will be easy to make this specialization call `::test<unsigned long long>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72743