Re: char-TPM: Adjustments for ten function implementations

[Michal Suchánek]

On Wed, 18 Oct 2017 02:18:46 -0700
Joe Perches  wrote:

> On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > The printk removals do change the objects.
> > > 
> > > The value of that type of change is only for resource limited
> > > systems.  
> > 
> > I imagine that such small code adjustments are also useful for
> > other systems.  
> 
> Your imagination and mine differ.
> Where do you _think_ it matters?
> 
> For instance, nothing about
> 
>   sizeof(type)
> vs
>   sizeof(*ptr)
> 
> makes it easier for a human to read the code.
> 

However, it makes it less error-prone to modify the code.

If you do ptr = malloc(sizeof(*ptr)) and later you change the type of
the pointer the code is still correct whereas ptr = malloc(sizeof(some
type) no longer is.

That is the reason the source analysis tool warns about this usage and
you do not really need any more explanation for *this* change.

The others are not so clear.

Thanks

Michal

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>>> A minor complaint: all commits are missing "Fixes:" tag.
>>
>> * Do you require it to be added to the commit messages?
> 
> I don't require it. It's part of the development process:
> 
> https://www.kernel.org/doc/html/v4.12/process/submitting-patches.html

Yes. - But other contributors pointed the detail out again
that not every change is qualified for using this tag.


>> * Would you like to get a finer patch granularity then?
> 
> I don't understand what you are asking.

If you would insist on the addition of this tag to all my commits
for the discussed patch series, I imagine that I would need to split
the update step “Improve a size determination in nine functions”
into smaller parts.


>> * Do you find any more information missing?
> 
> I think I already answered to this in my earlier responses
> (commit messages).

Partly.


> I probably won't take "sizeof(*foo)" type of change even if it
> is a recommended style if that is the only useful thing that the
> commit does.

How much do you care for the section “14) Allocating memory”
in the document “coding-style.rst” then?

Regards,
Markus

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 2017-10-18 at 10:44 +, alexander.stef...@infineon.com wrote:
> > > For instance, nothing about
> > > > >   sizeof(type)
> > > > > vs
> > > > >   sizeof(*ptr)
> > > > > makes it easier for a human to read the code.
> > > >
> > > > If it does not make it easier to read the code for you, then maybe you
> > > > should consider that this might not be true for all humans. For me, it
> > > > makes it much easier to see at a glance, that code like
> > > > ptr=malloc(sizeof(*ptr)) is correct.
> > >
> > > I don't think there is a perfect solution.
> >
> > Maybe. But for the second variant the correctness is easier to check,
> 
> How often should
>   ptr = alloc(sizeof(*ptr))
> be
>   ptr = alloc(sizeof(**ptr))

Never? Because in that case it probably should be *ptr=alloc(sizeof(**ptr)), 
unless you are doing something horrible ;-)

Alexander

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 10:44 +, alexander.stef...@infineon.com wrote:
> > For instance, nothing about
> > > > sizeof(type)
> > > > vs
> > > > sizeof(*ptr)
> > > > makes it easier for a human to read the code.
> > > 
> > > If it does not make it easier to read the code for you, then maybe you
> > > should consider that this might not be true for all humans. For me, it
> > > makes it much easier to see at a glance, that code like
> > > ptr=malloc(sizeof(*ptr)) is correct.
> > 
> > I don't think there is a perfect solution.
> 
> Maybe. But for the second variant the correctness is easier to check,

How often should
ptr = alloc(sizeof(*ptr))
be
ptr = alloc(sizeof(**ptr))

>  both mentally and programmatically, because there is no need for any context 
> (the type of ptr does not matter).

Context matters.

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 18 Oct 2017, alexander.stef...@infineon.com wrote:
> 
> > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > > The printk removals do change the objects.
> > > > >
> > > > > The value of that type of change is only for resource limited systems.
> > > >
> > > > I imagine that such small code adjustments are also useful for other
> > > systems.
> > >
> > > Your imagination and mine differ.
> > > Where do you _think_ it matters?
> > >
> > > For instance, nothing about
> > >
> > >   sizeof(type)
> > > vs
> > >   sizeof(*ptr)
> > >
> > > makes it easier for a human to read the code.
> >
> > If it does not make it easier to read the code for you, then maybe you
> > should consider that this might not be true for all humans. For me, it
> > makes it much easier to see at a glance, that code like
> > ptr=malloc(sizeof(*ptr)) is correct.
> 
> I don't think there is a perfect solution.

Maybe. But for the second variant the correctness is easier to check, both 
mentally and programmatically, because there is no need for any context (the 
type of ptr does not matter).

> The type argument to sizeof
> could have the wrong type.  The expression argument to sizeof could be
> missing the *.  Unpleasant consequences are possible in both cases.
> Probably each maintainer has a style they prefer.  Perhaps it could be
> useful to adjust the code to follow the dominant strategy, in cases where
> there are a inconsistencies.

Certainly. At least within a file, there should be only one style.

> For example
> 
> if (...)
>   x = foo1(sizeof(struct xtype));
> else
>   x = foo2(sizeof(*x));
> 
> might at least cause some unnecessary mental effort to process.
> 
> julia

Alexander

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 12:00 +0200, Julia Lawall wrote:
> 
> On Wed, 18 Oct 2017, alexander.stef...@infineon.com wrote:
> 
> > > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > > The printk removals do change the objects.
> > > > > 
> > > > > The value of that type of change is only for resource limited systems.
> > > > 
> > > > I imagine that such small code adjustments are also useful for other
> > > 
> > > systems.
> > > 
> > > Your imagination and mine differ.
> > > Where do you _think_ it matters?
> > > 
> > > For instance, nothing about
> > > 
> > >   sizeof(type)
> > > vs
> > >   sizeof(*ptr)
> > > 
> > > makes it easier for a human to read the code.
> > 
> > If it does not make it easier to read the code for you, then maybe you
> > should consider that this might not be true for all humans. For me, it
> > makes it much easier to see at a glance, that code like
> > ptr=malloc(sizeof(*ptr)) is correct.
> 
> I don't think there is a perfect solution.  The type argument to sizeof
> could have the wrong type.  The expression argument to sizeof could be
> missing the *.

Yup.

Today, even after all of Markus' patches for this style
conversion, there is still only ~2:1 preference for
ptr = k.alloc(sizeof(*ptr))
over
ptr = k.alloc(sizeof(struct foo))
in the kernel tree

Ugly grep follows:

$ grep -rohP --include=*.[ch] "\w+\s*=\s*[kv].alloc\s*\(\s*sizeof.*," * | \
  sed -r -e 's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*\*\s*\1\s*\)/foo = 
k.alloc(sizeof(*foo))/' \
 -e 
's/(\w+)\s*=\s*[kv].alloc\s*\(\s*sizeof\s*\(\s*struct\s+\w+\s*\)/foo = 
k.alloc(sizeof(struct foo))/' | \
  sort | uniq -c | sort -rn | head -2
   6123 foo = k.alloc(sizeof(*foo)),
   3060 foo = k.alloc(sizeof(struct foo)),

> Unpleasant consequences are possible in both cases.

Yup.

> Probably each maintainer has a style they prefer.  Perhaps it could be
> useful to adjust the code to follow the dominant strategy, in cases where
> there are a inconsistencies.  For example
> 
> if (...)
>   x = foo1(sizeof(struct xtype));
> else
>   x = foo2(sizeof(*x));
> 
> might at least cause some unnecessary mental effort to process.

Sure, but perhaps _only_ when there are inconsistencies
in the same compilation unit.'

RE: char-TPM: Adjustments for ten function implementations

[Julia Lawall]

On Wed, 18 Oct 2017, alexander.stef...@infineon.com wrote:

> > On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > > The printk removals do change the objects.
> > > >
> > > > The value of that type of change is only for resource limited systems.
> > >
> > > I imagine that such small code adjustments are also useful for other
> > systems.
> >
> > Your imagination and mine differ.
> > Where do you _think_ it matters?
> >
> > For instance, nothing about
> >
> > sizeof(type)
> > vs
> > sizeof(*ptr)
> >
> > makes it easier for a human to read the code.
>
> If it does not make it easier to read the code for you, then maybe you
> should consider that this might not be true for all humans. For me, it
> makes it much easier to see at a glance, that code like
> ptr=malloc(sizeof(*ptr)) is correct.

I don't think there is a perfect solution.  The type argument to sizeof
could have the wrong type.  The expression argument to sizeof could be
missing the *.  Unpleasant consequences are possible in both cases.
Probably each maintainer has a style they prefer.  Perhaps it could be
useful to adjust the code to follow the dominant strategy, in cases where
there are a inconsistencies.  For example

if (...)
  x = foo1(sizeof(struct xtype));
else
  x = foo2(sizeof(*x));

might at least cause some unnecessary mental effort to process.

julia

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

>> I imagine that such small code adjustments are also useful for other systems.
> 
> Your imagination and mine differ.

This can generally be.


> Where do you _think_ it matters?

It seems that this discussion branch referred still to my cover letter
for possible changes in the TPM software area.

The four update steps (in this patch series) demonstrate different
change possibilities which could be desired.
Would you like to distinguish them a bit more?


> For instance, nothing about
> 
>   sizeof(type)
> vs
>   sizeof(*ptr)
> 
> makes it easier for a human to read the code.

I could agree to this view (in the general short form).
But nine statements became shorter in the concrete update suggestion
so that such a reduction could help the trained eyes
of some software developers and code reviewers.


> This class of change now require a syntactic parser
> to find instances of the use of type where previously
> a grep or equivalent tool worked well.

Does the Linux coding style convention prefer safety over this
data processing concern?


>>> Markus' changelogs leave much to be desired.
>>
>> Would you like to help more to improve the provided information
>> for the shown change patterns?
> 
> I've done that for you far too many times already.

I got an other impression.
You gave constructive feedback (also for me) occasionally.

There were a few cases where a desired agreement was not achieved so far.


> Your changelogs need to detail _why_ something is being done,

I could improve descriptions if involved information sources
could also become clearer and really safe.


> not describe any tool used to perform or find a
> particular instance of any change.

This part refers to a bit of attribution.

Regards,
Markus

RE: char-TPM: Adjustments for ten function implementations

[Alexander.Steffen]

> On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > > The printk removals do change the objects.
> > >
> > > The value of that type of change is only for resource limited systems.
> >
> > I imagine that such small code adjustments are also useful for other
> systems.
> 
> Your imagination and mine differ.
> Where do you _think_ it matters?
> 
> For instance, nothing about
> 
>   sizeof(type)
> vs
>   sizeof(*ptr)
> 
> makes it easier for a human to read the code.

If it does not make it easier to read the code for you, then maybe you should 
consider that this might not be true for all humans. For me, it makes it much 
easier to see at a glance, that code like ptr=malloc(sizeof(*ptr)) is correct.

Alexander

Re: char-TPM: Adjustments for ten function implementations

[Joe Perches]

On Wed, 2017-10-18 at 11:00 +0200, SF Markus Elfring wrote:
> > The printk removals do change the objects.
> > 
> > The value of that type of change is only for resource limited systems.
> 
> I imagine that such small code adjustments are also useful for other systems.

Your imagination and mine differ.
Where do you _think_ it matters?

For instance, nothing about

sizeof(type)
vs
sizeof(*ptr)

makes it easier for a human to read the code.

This class of change now require a syntactic parser
to find instances of the use of type where previously
a grep or equivalent tool worked well.

> > Markus' changelogs leave much to be desired.
> 
> Would you like to help more to improve the provided information
> for the shown change patterns?

I've done that for you far too many times already.

Your changelogs need to detail _why_ something is being
done, not describe any tool used to perform or find a
particular instance of any change.

Re: char-TPM: Adjustments for ten function implementations

[SF Markus Elfring]

> The printk removals do change the objects.
> 
> The value of that type of change is only for resource limited systems.

I imagine that such small code adjustments are also useful for other systems.


> Markus' changelogs leave much to be desired.

Would you like to help more to improve the provided information
for the shown change patterns?

Regards,
Markus