sammccall added inline comments.
================ Comment at: llvm/include/llvm/Testing/Support/Error.h:168 +/// Helper marcro for checking the result of an 'Expected<T>' +/// ---------------- marcro -> macro ================ Comment at: llvm/include/llvm/Testing/Support/Error.h:179 +/// // calling 'toString()'. This also helps in debugging failing tests. +/// EXPECT_THAT_EXPECTED(D1, Succeeded()) << toString(D1.takeError()) << +/// "\n"; ---------------- you shouldn't explicitly log the tostring, this should happen automatically ================ Comment at: llvm/include/llvm/Testing/Support/Error.h:181 +/// "\n"; +/// EXPECT_THAT(*D1, Eq(2)); +/// ---------------- this is unidiomatic/incorrect, rather just `EXPECT_THAT_EXPECTED(D1, HasValue(2))` in one step (Note that if you want to write `*D1`, you need to establish that `D1` succeeded, e.g. by using `ASSERT_` to bail out early. As it is, if there is an error it'll show one failure and then crash) ================ Comment at: llvm/include/llvm/Testing/Support/Error.h:187 +/// // In the error case we need to consume the error. +/// consumeError(D2.takeError()); +/// } ---------------- no, you don't! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105014/new/ https://reviews.llvm.org/D105014 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
