Re: [PATCH v3 03/12] object-file API: add a format_object_header() function
- From
Jiang Xin <worldhello.net@gmail.com>
- Date
- Mar 1, 2022, 03:09 UTC
- Message-ID
- <CANYiYbFZN=mvcxRT4Cf4Gzao89KR2UjMJXnJM63S2zAtbD32Jg@mail.gmail.com>
- In-Reply-To
- <220217.86mtipj14m.gmgdl@evledraar.gmail.com>
On Thu, Feb 17, 2022 at 5:25 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 48 quoted lines
>
>
> On Thu, Feb 17 2022, Jiang Xin wrote:
>
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> >
> > Type casting can be avoid if we use "void *" as the first parameter of
> > "format_object_header", and do type casting inside the helper function.
> > [...]
> > The return value of `type_name(type)` has not been checked for the original
> > implement, how about write a online inline-function in a header file like this:
> >
> > static inline int format_object_header(void *str, size_t size,
> > const char *type_name,
> > size_t objsize)
> > {
> > return xsnprintf((char *)str, size, "%s %"PRIuMAX, type_name,
> > (uintmax_t)objsize) + 1;
> > }
>
> I don't think the casting in the callers is bad, in that for the callers
> that do use "char *" we get the compiler to help us with type checking.
>
> Using a void * is something we really reserve only for callback-type
> values, because it mens that now nobody gets any type checking.
>
> I think if we wanted to avoid the casts it would make more sense to add
> a trivial ucformat_object_header() wrapper or whatever, which would take
> "unsigned chan *" and do the cast, or just tweak the relevant calling
> code to change the type (IIRC some of it used unsigned v.s. signed for
> no particular reason).
>
> But I think just leaving this part as it is is better here...
>
> > [...]
> >> + if (!name)
> >> + BUG("could not get a type name for 'enum object_type' value %d", type);
> >> +
> >
> > The return value of `type_name(type)` has not been checked for the original
> > implement, how about write a online inline-function in a header file like this:
>
> Yes, this part is not a faithful conversion on my part, but I think it
> made sense when converting this to a library function.
>
> The alternative is that we'd segfault on some platforms (not glibc,
> since it's OK with null %s arguments), just checking it is cheap & I
> think a good sanity check...After sending the last email, I was offline for over a week due to an emergency at home.
According to the rest patches of this series, your implementation of "format_object_header()" which uses "enum object_type" as the parameter instead of "char *type" is better. And adding a validation for `type_name(type)` is also better than my one-line inline function. This series of patches LGTM.
-- Jiang Xin