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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra marked an inline comment as done.
sivachandra added a comment.
This revision is now accepted and ready to land.

Couple of really nit-picky comments but overall LGTM.



================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:13
+#include "Test.h"
+#include "src/errno/llvmlibc_errno.h"
+
----------------
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?


================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:53
+
+namespace ErrnoSetterMatcher {
+
----------------
abrachet wrote:
> > These names are generic enough that some kind of specificity would be nice? I am not able to think of an alternate. May be ErrnoSetterSucceeds and ErrnoSetterFails?
> 
> What about this set up? I think it is nice to be able to use a `using` and refer to these as just `Succeeds` and `Fails` for brevity. No strong preference happy to call these `ErrnoSetterSucceeds/Fails`
LGTM


================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:56
+template <typename RetT = int>
+static internal::ErrnoSetterMatcher<RetT> Succeeds(RetT ExpectedReturn = 0,
+                                                   int ExpectedErrno = 0) {
----------------
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?


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

https://reviews.llvm.org/D75487