On Sat, Oct 1, 2022 at 11:50 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
On Fri, Sep 30, 2022 at 8:26 PM David Gow davidgow@google.com wrote:
On Sat, Oct 1, 2022 at 8:26 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
Context: Currently this macro's name, KUNIT_ASSERTION conflicts with the name of an enum whose values are {KUNIT_EXPECTATION, KUNIT_ASSERTION}.
It's hard to think of a better name for the enum, so rename this macro. It's also a bit strange that the macro might do nothing depending on the boolean argument `pass`. Why not have callers check themselves?
This patch: Moves the pass/fail checking into the callers of KUNIT_ASSERTION, so now we only call it when the check has failed. Then we rename the macro the _KUNIT_FAILED() to reflect the new semantics.
Signed-off-by: Daniel Latypov dlatypov@google.com
Looks good to me. I can't say the name _KUNIT_FAILED() feels perfect to me, but I can't think of anything better, either. We've not used a leading underscore for internal macros much thus far, as well, though I've no personal objections to starting.
Yeah, I also didn't add a leading underscore on the new KUNIT_INIT_ASSERT() macro elsewhere in this series. So I'm not necessarily proposing that we should start doing so here.
Yeah: I noticed that. In general, I think I come down slightly on the side of avoiding leading underscores. (And there's also the debate about whether to have one or two, as while two underscores is nominally "reserved for the system", the kernel uses it a lot -- probably because it considers itself "the system"). So I'd tend to lean away from making all of our "internal" macros start with underscores.
It feels like that KUNIT_FAILED is far too similar to the enum 55 enum kunit_status { 56 KUNIT_SUCCESS, 57 KUNIT_FAILURE, 58 KUNIT_SKIPPED, 59 };
I.e. we'd be remove one naming conflict between a macro and enum, but basically introducing a new one in its place :P Tbh, I was originally going to have this patch just be s/KUNIT_ASSERTION()/_KUNIT_ASSERTION() to reduce the conflict. But I figured we could reduce the number of arguments to the macro (drop `pass`) and have a reason to give it a different name.
I'm also not entirely convinced about _KUNIT_FAILED(), but I haven't had any significantly better ideas since I sent the RFC in May.
Agreed: particularly since SKIPPED would seem to pair better with FAILED than FAILURE, so they conflict quite a bit.
Let's stick with what we have in this change, and we can change it later if someone comes up with a drastically better name.
Cheers, -- David