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

Thu Jul 16 23:55:59 PDT 2020

sivachandra added inline comments.
Herald added a subscriber: ecnelises.

================
Comment at: libc/utils/UnitTest/ErrnoSetterMatcher.h:13
+#include "Test.h"
+#include "src/errno/llvmlibc_errno.h"
+
----------------
sivachandra wrote:
> 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.
I am beginning to feel that your suggestion is more appropriate. For, the current arrangement breaks a layering rule: a part of `utils` depends on `src`. We do not want `utils` to depend on `src`.

To keep the directory structure of `test` match that of the the libc tree, we can take one of these two options:
1. Move `testutils` up out of `utils` in to a top level directory of its own.
2. Move just this file into `test` and not nest it under `test/common`.

I do not have an strong leanings, but it seems like #2 can be less confusing. WDYT?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75487