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

[Siva Chandra via Phabricator via libc-commits]

sivachandra marked 2 inline comments as done.
sivachandra added inline comments.


================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:13
+#include "Test.h"
+#include "src/errno/llvmlibc_errno.h"
+
----------------
abrachet wrote:
> 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?
No, leave it as is but add a comment. We want `test/...` to reflect the directory tree of the rest of the repo. If we really want a separate place, then may be `utils/testmatchers`.  However, it feels like we should wait for more examples before we move things around.


================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:56
+template <typename RetT = int>
+static internal::ErrnoSetterMatcher<RetT> Succeeds(RetT ExpectedReturn = 0,
+                                                   int ExpectedErrno = 0) {
----------------
abrachet wrote:
> 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.
SGTM. Worst case, we can easily fix this if it gets irritating in future.


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

https://reviews.llvm.org/D75487