[libc][NFC] Extend <ASSERT|EXPECT>_STR* macros to compare with nullptr.

[sivachandra]

Summary

In future, pointer comparison might also need to be extended to compare
with nullptr values. That can be taken up when needed.

[abrachet]

libc/utils/UnitTest/Test.cpp

245–246

These should use internal::test(Ctx, Cond_EQ, LHS, RHS, ...) so we get nice error messages and are actually reported as failures.

[sivachandra]

libc/utils/UnitTest/Test.cpp

245–246

Huh! I am not sure why this diff got uploaded. I am on a different machine today and probably did arc diff without committing the changes. I used this diff to find tests which would fail but I have a made more changes over it. Will upload the right diff now.

[sivachandra]

Correct diff (hopefully) with a fix to strstr included.

[sivachandra]

@cgyurgyik - PTAL. The latest diff looks correct to me (as in, what I intended to upload) and includes a fix to strstr. Without the fix, a strstr test fails with the nullptr change to the tests.

[cgyurgyik]

In D85790#2211882, @sivachandra wrote:

@cgyurgyik - PTAL. The latest diff looks correct to me (as in, what I intended to upload) and includes a fix to strstr. Without the fix, a strstr test fails with the nullptr change to the tests.

strstr change LGTM. Not familiar enough with test suite to make that call so I'll let abrachet LGTM this.

[sivachandra]

In D85790#2211927, @cgyurgyik wrote:

In D85790#2211882, @sivachandra wrote:

@cgyurgyik - PTAL. The latest diff looks correct to me (as in, what I intended to upload) and includes a fix to strstr. Without the fix, a strstr test fails with the nullptr change to the tests.

strstr change LGTM. Not familiar enough with test suite to make that call so I'll let abrachet LGTM this.

Thanks.

@abrachet, I think we can do something smarter here. For example, I don't like using "<nullptr>". But I am not sure if the complexity is worth it. If required, we can always extend this. Feel free to let me know if you can think of a better approach without too much of moving things around.

[cgyurgyik]

libc/utils/UnitTest/Test.cpp

256

So if I'm not mistaken, this will just compare "<nullptr>" with the other string, which will (falsely) return true if the other string is also the string literal "<nullptr>"? Maybe we can put something else a bit longer, such as ___<nullptr>___ ?
Or, just simply check here if the other string is actually the string literal "<nullptr>" to catch that case.

Just blanket suggestions, please let me know if I'm mistaken here.

[sivachandra]

libc/utils/UnitTest/Test.cpp

256

No you are not mistaken. And yes, using a "default" string is not perfect. Other things like you suggest (check iff the real string is equal to the actual string) etc can be done but not sure if that is worth it. I would ideally want llvm::StringRef to take of this. But, it is very specifically designed to allow equal comparison of null and nullpointer: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/ADT/StringRef.h#L74

The best solution would be to use a class with has std::string_view semantics.

[cgyurgyik]

libc/utils/UnitTest/Test.cpp

256

Got it. Thanks for the insight.

[sivachandra]

libc/utils/UnitTest/Test.cpp

256

I am wrong: std::string_view cannot handle nullptr values. May the best solution is to implement a standalone equivalent of std::string_view with slightly differing semantics. Not sure if it is all worth it at this time.

[abrachet]

libc/utils/UnitTest/Test.cpp

256

What about

struct StrCompare : public llvm::StringRef {
   bool operator==(llvm::StringRef other) {
       if (!data() || !other.data()) return data() == other.data();
       return other == *this;
   }
};

And pass StrCompare(LHS) instead of llvm::StringRef(LHS)

But I think "<nullptr>" works too. We are never going to have the string "<nullptr>" so it's not worth worrying about.

libc/src/string/strstr.cpp

 #include "src/__support/common.h"
 #include <stddef.h>
 
 namespace __llvm_libc {
 
 // TODO: This is a simple brute force implementation. This can be
 // improved upon using well known string matching algorithms.
 char *LLVM_LIBC_ENTRYPOINT(strstr)(const char *haystack, const char *needle) {
+  // The standard requires that haystack is returned if the needle is an empty
+  // string.
+  if (*needle == '\0')
+    return const_cast<char *>(haystack);
+
   for (size_t i = 0; haystack[i]; ++i) {
     size_t j;
     for (j = 0; haystack[i + j] && haystack[i + j] == needle[j]; ++j)
       ;
     if (!needle[j])
       return const_cast<char *>(haystack + i);
   }
   return nullptr;
 }
 
 } // namespace __llvm_libc

libc/utils/UnitTest/Test.cpp

 template bool Test::test<__uint128_t, 0>(RunContext &Ctx, TestCondition Cond,
                                          __uint128_t LHS, __uint128_t RHS,
                                          const char *LHSStr, const char *RHSStr,
                                          const char *File, unsigned long Line);
 
 bool Test::testStrEq(RunContext &Ctx, const char *LHS, const char *RHS,
                      const char *LHSStr, const char *RHSStr, const char *File,
                      unsigned long Line) {
+  if (LHS == nullptr)
+    LHS = "<nullptr>";
+  if (RHS == nullptr)
+    RHS = "<nullptr>";
   return internal::test(Ctx, Cond_EQ, llvm::StringRef(LHS),
                         llvm::StringRef(RHS), LHSStr, RHSStr, File, Line);
 }
 
 bool Test::testStrNe(RunContext &Ctx, const char *LHS, const char *RHS,
                      const char *LHSStr, const char *RHSStr, const char *File,
                      unsigned long Line) {
+  if (LHS == nullptr)
+    LHS = "<nullptr>";
+  if (RHS == nullptr)
+    RHS = "<nullptr>";
   return internal::test(Ctx, Cond_NE, llvm::StringRef(LHS),
                         llvm::StringRef(RHS), LHSStr, RHSStr, File, Line);
 }
 
 bool Test::testMatch(RunContext &Ctx, bool MatchResult, MatcherBase &Matcher,
                      const char *LHSStr, const char *RHSStr, const char *File,
                      unsigned long Line) {
   if (MatchResult)