On Wed, Mar 19, 2025 at 05:08:10PM +0100, Miquel Raynal wrote:
> print_hex_dump() already has numerous parameters, and could be extended
> with a new one. Adding new parameters is super painful due to the number
> of users, and it makes the function calls even longer.
>
> Create a print_hex() to replace print_hex_dump(), with 'prefix_type' and
> 'ascii' being merged into a 'dump_flags' parameter. This way extending
> the list of dump flags will be much easier.
>
> For convenience, a print_hex_dump macro is created to fallback on the
print_hex_dump()
> print_hex() implementation. A tree-wide change to remove its use could
> be done in the future.
>
> No functional change intended.
...
> For printing small buffers (up to 64 bytes long) as a hex string with a
> certain separator. For larger buffers consider using
> -:c:func:`print_hex_dump`.
> +:c:func:`print_hex`.
Why replacement? I would rather expect
:c:func:`print_hex_dump` or :c:func:`print_hex` depending on your needs.
...
> +/*
> + * Dump flags for print_hex().
> + * DUMP_PREFIX_{NONE,ADDRESS,OFFSET} are mutually exclusive.
This is confusing, taking into account two definitions to 0.
> + */
> enum {
> + DUMP_HEX_DATA = 0,
> + DUMP_ASCII = BIT(0),
> + DUMP_PREFIX_NONE = 0, /* Legacy definition for print_hex_dump() */
> + DUMP_PREFIX_ADDRESS = BIT(1),
> + DUMP_PREFIX_OFFSET = BIT(2),
> };
Can we rather add a new enum and leave this untouched?
Also you can use bit mask and two bits for the value:
DUMP_PREFIX_MASK = GENMASK(1, 0)
and no need to have the above comment about exclusiveness and no need to change
the values.
...
> +extern void print_hex(const char *level, const char *prefix_str,
> + int rowsize, int groupsize,
> + const void *buf, size_t len,
> + unsigned int dump_flags);
> +static inline void print_hex(const char *level, const char *prefix_str,
> + int rowsize, int groupsize,
> + const void *buf, size_t len,
> + unsigned int dump_flags)
Hmm... Wouldn't you want to have a enum as a last parameter?
--
With Best Regards,
Andy Shevchenko
