Re: [PATCH v3 03/12] object-file API: add a format_object_header() function

[Jiang Xin <worldhello.net@gmail.com>]

On Thu, Feb 17, 2022 at 5:25 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> 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