[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 a comment.

In D72743#1825799 <https://reviews.llvm.org/D72743#1825799>, @abrachet wrote:

> I just checked quickly and it looks like gtest actually prints to stdout which surprised me, I always figured it was to stderr. Do you think we should go back to stdout?


TBF, I do not have any opinion here. For me, a message from a test run is all useful. So, its hard for me to see why something should be logged to stderr and something else to stdout. I have now put everything back to stdout because I actually think there could be tools out there which are probably only looking at stdout.



================
Comment at: libc/utils/UnitTest/Test.cpp:148
+    const char *TestName = T->getName();
+    llvm::outs() << "[ RUN      ] " << TestName << '\n';
+    RunContext Ctx;
----------------
abrachet wrote:
> This one is still to `outs`
The others were really an error message, so may be stderr was more appropriate. These are purely informational and hence I left them with stdout.


================
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:
> 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 ]].
Ah, I never used this before. I have modified as you have suggested now.


================
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:
> 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>`.
I don't have any opinion about this. I had previously thought I should limit the re-implementation of std types. But heck, we have already come this far so I have implemented everything as you have suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72743