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

[Ilya Biryukov via Phabricator via llvm-commits]

ilya-biryukov added inline comments.


================
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:
----------------
sammccall wrote:
> 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...
Agreed.


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:85
+      if (!Input) {
+        *L << "does not have a value";
+        return false;
----------------
sammccall wrote:
> 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.
Agree, that should be fine


================
Comment at: llvm/include/llvm/Testing/Support/SupportHelpers.h:109
+template <class InnerMatcher>
+ValueIsMatcher<InnerMatcher> ValueIs(const InnerMatcher &ValueMatcher) {
+  return ValueIsMatcher<InnerMatcher>(ValueMatcher);
----------------
sammccall wrote:
> 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...".
Totally agree, couldn't come up with a better name that wasn't taken.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61071