[PATCH] D43330: [gtest] Add PrintTo overload for StringRef.

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

In https://reviews.llvm.org/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


Repository:
  rL LLVM

https://reviews.llvm.org/D43330