On Wed 01-09-21 11:21:49, Oleksandr Natalenko wrote:
> There are various places where the K(x) macro is defined. This commit
> gets rid of multiple definitions and provides a common one.
>
> This doesn't solve open-coding this macro in various other places. This
> should be addressed by another subsequent commit.
Why is this an improvement? You are adding a header file for a single
macro which sounds like an overkill. The overall net outcome is added
lines of code. It is not like K() or any of its variant is adding a
maintenance burden due to code duplication. So why do we want to change
the existing state?
--
Michal Hocko
SUSE Labs
Hello.
On středa 1. září 2021 12:31:36 CEST Michal Hocko wrote:
> On Wed 01-09-21 11:21:49, Oleksandr Natalenko wrote:
> > There are various places where the K(x) macro is defined. This commit
> > gets rid of multiple definitions and provides a common one.
> >
> > This doesn't solve open-coding this macro in various other places. This
> > should be addressed by another subsequent commit.
>
> Why is this an improvement? You are adding a header file for a single
> macro which sounds like an overkill.
I agree a separate header file is an overkill for just one #define, hence
still looking for a suggestion on a better place for it.
> The overall net outcome is added
> lines of code.
Not always. There are some long statements like:
```
seq_printf(seq, ",size=%luk",
sbinfo->max_blocks << (PAGE_SHIFT - 10));
```
that are split into two lines. With the macro those take one line only:
```
seq_printf(seq, ",size=%luk", K(sbinfo->max_blocks));
```
As of now (counting unposted open-coding replacements) the grand total is:
```
31 files changed, 104 insertions(+), 90 deletions(-)
```
which is not that horrible.
> It is not like K() or any of its variant is adding a
> maintenance burden due to code duplication. So why do we want to change
> the existing state?
For me it's about readability. Compare, for instance:
```
seq_put_decimal_ull_width(m, str, (val) << (PAGE_SHIFT-10), 8)
```
and
```
seq_put_decimal_ull_width(m, str, K(val), 8)
```
It's a small yet visible difference.
Thanks.
--
Oleksandr Natalenko (post-factum)
On Wed 01-09-21 12:50:40, Oleksandr Natalenko wrote:
[...]
> ```
> 31 files changed, 104 insertions(+), 90 deletions(-)
> ```
>
> which is not that horrible.
Still a lot of churn to my taste for something that is likely a matter
of personal preferences and taste. Consider additional costs as well.
E.g. go over additional git blame steps to learn why the code has been
introduced, review bandwith etc...
And just be clear, I am not really opposing this patch I just do not see
a justification and in general I am not super thrilled about cleanups
which are not really necessary for a bigger goal - exactly because of
the additional costs mentioned above.
--
Michal Hocko
SUSE Labs
From: Michal Hocko
> Sent: 01 September 2021 12:11
>
> On Wed 01-09-21 12:50:40, Oleksandr Natalenko wrote:
> [...]
> > ```
> > 31 files changed, 104 insertions(+), 90 deletions(-)
> > ```
> >
> > which is not that horrible.
>
> Still a lot of churn to my taste for something that is likely a matter
> of personal preferences and taste. Consider additional costs as well.
> E.g. go over additional git blame steps to learn why the code has been
> introduced, review bandwith etc...
Not to mention the time taken by someone scan-reading the
code who has to go and find the definition of K() just
to see what it does.
A more descriptive name (eg PAGES_TO_KB) might save that.
but is it worth it?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)