[libc-commits] [PATCH] D75487: [libc] [UnitTest] Add Matchers

[Alex Brachet via Phabricator via libc-commits]

abrachet marked 4 inline comments as done.
abrachet added inline comments.


================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:13
+#include "Test.h"
+#include "src/errno/llvmlibc_errno.h"
+
----------------
sivachandra wrote:
> Normally, we should not use parts of the libc to build its testing infrastructure. This change is not doing anything of that sort, but having this include here gives an impression that we are using the libc to build the test infrastructure. So, may be add a comment of some kind to make it absolutely clear?
Yes I was definitely weary about this. I had originally planned to put this file in test/ not utils/UnitTest but couldn't find a good place. Maybe making a common directory in test? So in libc/test/common/ErrnoSetterMatcher.h? Does that sound better?


================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:56
+template <typename RetT = int>
+static internal::ErrnoSetterMatcher<RetT> Succeeds(RetT ExpectedReturn = 0,
+                                                   int ExpectedErrno = 0) {
----------------
sivachandra wrote:
> Sorry I missed pointing out earlier: why not make the order of args to `Fail` and `Succeeds` the same? Even to the constructor of ErrnoSetterMatcher above?
I agree it is confusing.

The idea here was that for a failure errno is always different, and it would be awkward to do `Fails(/*ExpectedReturn*/-1, EINVAL)` rather than `Fails(EINVAL)`. Likewise for `Success(/*ExpectedErrno*/0, /*ExpectedReturn*/0)` is awkward because errno should always be 0. Now that I think about it, maybe `Success` shouldn't take an errno parameter at all I can't imagine it ever being useful.

My plan is for these to be able to take matchers of their own in a future patch. I could in this patch have these take matchers and create an `ErrnoIs`/`ReturnIs` if you think the ordering of arguments is confusing enough to warrant this.


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

https://reviews.llvm.org/D75487