Re: [PATCH 2/4] kunit: rename base KUNIT_ASSERTION macro to _KUNIT_FAILED

[David Gow <davidgow@google.com>]

[-- Attachment #1: Type: text/plain, Size: 2859 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]