From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Petr Mladek <pmladek@suse.com>,
David Laight <david.laight.linux@gmail.com>,
Steven Rostedt <rostedt@goodmis.org>,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
Sergey Senozhatsky <senozhatsky@chromium.org>,
Jonathan Corbet <corbet@lwn.net>,
John Ogness <john.ogness@linutronix.de>,
Andrew Morton <akpm@linux-foundation.org>,
Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 2/3] hexdump: Allow skipping identical lines
Date: Wed, 19 Mar 2025 19:08:04 +0100 [thread overview]
Message-ID: <87tt7oyjuj.fsf@bootlin.com> (raw)
In-Reply-To: <Z9rzy9WQcy_MgH9v@smile.fi.intel.com> (Andy Shevchenko's message of "Wed, 19 Mar 2025 18:41:47 +0200")
>> 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`.
>
> Instead of fixing this (see also comment in previous patch), just add the text
> like
>
> :c:func:`print_hex` is especially useful since duplicated lines can be skipped
> automatically to reduce the overhead with the
> ``DUMP_SKIP_IDENTICAL_LINES`` flag.
Why are you bothered here? I am sorry but this part of the review is
absolutely pointless. I have no words to emphasize how high the
nitpicking level is. Please refrain yourself.
>> +:c:func:`print_hex`, especially since duplicated lines can be
>> +skipped automatically to reduce the overhead with the
>> +``DUMP_SKIP_IDENTICAL_LINES`` flag.
>
> Also, can we also put a sub name spaces to the flags, like for HEX/ASCII
>
> DUMP_DATA_HEX
> DUMP_DATA_ASCII
They just refer to the way the data is dumped, so "data" is in the name,
that's true. The fact that they share the same namespace is fortunate
but not super relevant either. I am following the hints discussed in v2
regarding the naming, I am not too attached to these, but they felt
correct, so I use them.
> This SKIP will start a new sub name space.
Yes. And?
I would have probably chosen a common name space from day 1 if I was
creating these now, but they are already two enums named
"DUMP_PREFIX". I guess we all agree it would be stupid to prefix all
enums "DUMP_PREFIX" anyway? And because you disgusted me from attempting
brave tree-wide changes, I will not rename them.
>
> ...
>
>> #include <linux/errno.h>
>> #include <linux/kernel.h>
>> #include <linux/minmax.h>
>
>> +#include <linux/string.h>
>> #include <linux/export.h>
>
> It's more natural to put it here, with given context it makes more order
> (speaking of alphabetical one).
If I learned something on these mailing lists, it is that what is
natural for me might not be for others. The alphabetical order is not
respected. You already pointed that in your first review, so I chose
another place which felt more relevant... to me.
e, k, m, s, u. This is the alphabetical order. export.h is not at the
correct place, I am very sorry.
next prev parent reply other threads:[~2025-03-19 18:08 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-03-19 16:08 [PATCH v3 0/3] hexdump: Allow skipping identical lines Miquel Raynal
2025-03-19 16:08 ` [PATCH v3 1/3] hexdump: Simplify print_hex_dump() Miquel Raynal
2025-03-19 16:37 ` Andy Shevchenko
2025-03-19 17:53 ` Miquel Raynal
2025-03-19 16:08 ` [PATCH v3 2/3] hexdump: Allow skipping identical lines Miquel Raynal
2025-03-19 16:41 ` Andy Shevchenko
2025-03-19 18:08 ` Miquel Raynal [this message]
2025-03-19 16:08 ` [PATCH v3 3/3] hexdump: Print the prefix after the last line to show the dump is over Miquel Raynal
2025-03-19 16:44 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87tt7oyjuj.fsf@bootlin.com \
--to=miquel.raynal@bootlin.com \
--cc=akpm@linux-foundation.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=corbet@lwn.net \
--cc=david.laight.linux@gmail.com \
--cc=john.ogness@linutronix.de \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=pmladek@suse.com \
--cc=rostedt@goodmis.org \
--cc=senozhatsky@chromium.org \
--cc=thomas.petazzoni@bootlin.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).