[gtest] Add PrintTo overload for StringRef.

[ilya-biryukov]

Summary

It was printed using code for generic containers before, resulting in
unreadable output.

[labath]

This code does not have to live directly inside gtest. As such, maybe a better place for it would be somewhere under include/llvm/Testing ? (We already have PrintTo functions for Error and Expected there..)

The reason I'm mentioning this is that while StringRef is a fairly basic class, I'm sure other classes would benefit from being GTEST-ified as well, and shoving them all somewhere into third party code sounds like a bad idea...

[ilya-biryukov]

In D43330#1008558, @labath wrote:

This code does not have to live directly inside gtest. As such, maybe a better place for it would be somewhere under include/llvm/Testing ? (We already have PrintTo functions for Error and Expected there..)

The reason I'm mentioning this is that while StringRef is a fairly basic class, I'm sure other classes would benefit from being GTEST-ified as well, and shoving them all somewhere into third party code sounds like a bad idea...

Thanks for mentioning those, I didn't know we had them.
Putting PrintTo(StringRef) into llvm/Testing would mean that clients will have to include it in order to get nice test output, otherwise they'll end up with the defaults and the code will compile, but the output for StringRef is currently totally unreadable, something like {'#'(11), 'i' (12), 'n' (13), 'c' (14)} (numbers represent the char codes).
I don't think the same is true for Expected and Error, they won't compile, so users will have to include the header in order to fix compile errors.

One alternative is putting the PrintTo function into StringRef.h, but that would mean we'll have to add unnecessary include to StringRef.h (<ostream>). So I would actually argue for keeping it in the gtest helpers. WDYT?

[sammccall]

FWIW I'm fine with putting it in StringRef.h or here, (custom/gtest-printers.his strictly better than here though).

While I like llvm/Testing in principle, the fact that the test compiles and runs fine if you forget to include it, and just gives terrible error messages when it starts failing, seems like a recipe for disaster.

[labath]

This is unfortunate, but it's kind of a consequence of how googletest implements it's universal printers. The reason why I don't think it's a good idea is that this is not a problem specific to StringRef, as any class can come out as giberrish if the user forgets to include the PrintTo function. I am not adamant about this, but I think the bar for modifying gtest code should be higher than this (it's trivial to add the right include if you see you're values aren't coming out right).

It's possible I am being too pedantic about this (if it were up to me, I would implement even the raw_ostream trickery outside of gtest with a SFINAE-ed templated PrintTo function), but I'd like to hear others have to say about this, (particulary @zturner, as he's the one who came up with the llvm/Testing idea).

[sammccall]

In D43330#1008638, @labath wrote:

This is unfortunate, but it's kind of a consequence of how googletest implements it's universal printers. The reason why I don't think it's a good idea is that this is not a problem specific to StringRef, as any class can come out as giberrish if the user forgets to include the PrintTo function.

This problem only applies to container classes that shouldn't be printed as containers. gtest itself special-cases string for this reason.

I am not adamant about this, but I think the bar for modifying gtest code should be higher than this

How do you feel about custom/gtest-printers.h? It's explicitly intended for site modifications like this.

(it's trivial to add the right include if you see you're values aren't coming out right).

The issue is you only see the values if the tests fail.
So your test passes, you check in the code, someone else's change breaks the test, and they get gibberish from $OBSCURE_BUILDBOT.

It's possible I am being too pedantic about this (if it were up to me, I would implement even the raw_ostream trickery outside of gtest with a SFINAE-ed templated PrintTo function)

The problem with that is PrintTo isn't called on all the relevant codepaths, only operator<< - see gtest-message.h. If you can make this work, please send a patch - I'm aware the current approach is pretty layer-violating. (As was the previous)

I'd like to hear others have to say about this, (particulary @zturner, as he's the one who came up with the llvm/Testing idea).

+1

[ilya-biryukov]

• Move PrintTo to gtest-printers.h

[ilya-biryukov]

In D43330#1008638, @labath wrote:

I'd like to hear others have to say about this, (particulary @zturner, as he's the one who came up with the llvm/Testing idea).

Happy to wait for more input on this change.

In the meantime, moved the new overload to custom/gtest-printers.h and checked it works as expected.

[sammccall]

LG, but good idea to wait on more input

utils/unittest/googletest/include/gtest/internal/custom/gtest-printers.h

48 ↗

(On Diff #134433)

I think this is dead code, gtest won't call in this case

[ilya-biryukov]

• Assert stream is set in PrintTo()

utils/unittest/googletest/include/gtest/internal/custom/gtest-printers.h

48 ↗

(On Diff #134433)

Replaced with assertion

[ilya-biryukov]

Do you think it would be okay to land this if we get no response from @zturner by Monday?
We can revert the commit if any concerns will be raised after that.

[labath]

Yea, it seems nobody is troubled by this except me. I withdraw my objection.

utils/unittest/googletest/include/gtest/internal/custom/raw-ostream.h

auto printable(const T &V) -> decltype(StreamSwitch<T>::printable(V)) {
   // We delegate to the trait, to allow partial specialization.
   return StreamSwitch<T>::printable(V);
 }
 } // namespace llvm_gtest
 
 // If raw_ostream support is enabled, we specialize for types with operator<<
 // that takes a raw_ostream.
 #if !GTEST_NO_LLVM_RAW_OSTREAM
-#include "llvm/Support/raw_ostream.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/raw_os_ostream.h"
+#include "llvm/Support/raw_ostream.h"
 #include <ostream>
 namespace llvm_gtest {
 
 // The printable() of a raw_ostream-enabled type T is a RawStreamProxy<T>.
 // It uses raw_os_ostream to write the wrapped value to a std::ostream.
 template <typename T>
 struct RawStreamProxy {
   const T& V;

 // the possible conversion by passing something convertible to const T& instead.
 template <typename T> struct ConvertibleTo { operator T(); };
 template <typename T>
 struct StreamSwitch<T, decltype((void)(std::declval<llvm::raw_ostream &>()
                                        << ConvertibleTo<const T &>()))> {
   static const RawStreamProxy<T> printable(const T &V) { return {V}; }
 };
 } // namespace llvm_gtest
-#endif  // !GTEST_NO_LLVM_RAW_OSTREAM
+
+namespace llvm {
+// Without this overload StringRef will be printed as a generic container.
+inline void PrintTo(const llvm::StringRef &Val, std::ostream *S) {
+  if (!S)
+    return;
+  llvm::raw_os_ostream OS(*S);
+  OS << Val;
+}
+} // namespace llvm
+#endif // !GTEST_NO_LLVM_RAW_OSTREAM
 
 #endif // GTEST_INCLUDE_GTEST_INTERNAL_CUSTOM_RAW_OSTREAM_H_