[libc-commits] [PATCH] D85790: [libc][NFC] Extend <ASSERT|EXPECT>_STR* macros to compare with nullptr.

[Alex Brachet via Phabricator via libc-commits]

abrachet accepted this revision.
abrachet added inline comments.


================
Comment at: libc/utils/UnitTest/Test.cpp:256
                      unsigned long Line) {
+  if (LHS == nullptr)
+    LHS = "<nullptr>";
----------------
cgyurgyik wrote:
> sivachandra wrote:
> > sivachandra wrote:
> > > cgyurgyik wrote:
> > > > 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.
> > > 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.
> > 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.
> Got it. Thanks for the insight.
What about

```lang=cpp
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85790