[PATCH] D61071: [Support] Add a GTest matcher for Optional<T>

[Sam McCall via Phabricator via llvm-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:65
+/// To match llvm::None you could use Eq(llvm::None).
+template <class InnerMatcher> class ValueIsMatcher {
+public:
----------------
AFAICS from other helpers, ValueIsMatcher should go in llvm::detail, and ValueIs should go in llvm.

unittest::getInputFileDirectory() reads well, but unittest::ValueIs(...) really doesn't...


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:85
+      if (!Input) {
+        *L << "does not have a value";
+        return false;
----------------
is this actually necessary? I think it's already going to print the value, which is always "None" - I'm not sure "None, which does not have a value" is clearer.


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:109
+template <class InnerMatcher>
+ValueIsMatcher<InnerMatcher> ValueIs(const InnerMatcher &ValueMatcher) {
+  return ValueIsMatcher<InnerMatcher>(ValueMatcher);
----------------
it's a shame the name `HasValue` is taken, as I think it would much more clearly communicate "has a value *and* the value is...".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61071