On Sat, 8 Jun 2019, Markus Elfring wrote:
> > +- e1 = devm_ioremap_resource(arg4, id);
> > ++ e1 = devm_platform_ioremap_resource(arg1, arg3);
>
> Can the following specification variant matter for the shown SmPL
> change approach?
>
> + e1 =
> +- devm_ioremap_resource(arg4, id
> ++ devm_platform_ioremap_resource(arg1, arg3
> + );
In the latter case, the original formatting of e1 will be preserved. But
there is not usually any interesting formatting on the left side of an
assignment (ie typically no newlines or comments). I can see no purpose
to factorizing the right parenthesis.
julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

>>> +- e1 = devm_ioremap_resource(arg4, id);
>>> ++ e1 = devm_platform_ioremap_resource(arg1, arg3);
>>
>> Can the following specification variant matter for the shown SmPL
>> change approach?
>>
>> + e1 =
>> +- devm_ioremap_resource(arg4, id
>> ++ devm_platform_ioremap_resource(arg1, arg3
>> + );
>
> In the latter case, the original formatting of e1 will be preserved.
I would like to point the possibility out to express only required changes
also by SmPL specifications.
> But there is not usually any interesting formatting on the left side of an
> assignment (ie typically no newlines or comments).
Is there any need to trigger additional source code reformatting?
> I can see no purpose to factorizing the right parenthesis.
These characters at the end of such a function call should be kept unchanged.
I got another software development concern according to the discussed
software update “drivers: provide devm_platform_ioremap_resource()”
(from 2019-02-21).
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/platform.c?id=7945f929f1a77a1c8887a97ca07f87626858ff42
The flag “IORESOURCE_MEM” is passed as the second parameter for the call
of the function “platform_get_resource” in this refactoring.
Should this detail be specified also in the proposed script for the
semantic patch language instead of using the metavariable “arg2”
in SmPL disjunctions?
How do you think about to delete error detection and corresponding
exception handling code for the previous function call?
Is the SmPL code specification “when != id” really sufficient for
the exclusion of variable reassignments here?
Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

On 09.06.19 10:55, Markus Elfring wrote:
<snip>
>> But there is not usually any interesting formatting on the left side of an
>> assignment (ie typically no newlines or comments).
>
> Is there any need to trigger additional source code reformatting?
>
>> I can see no purpose to factorizing the right parenthesis.
>
> These characters at the end of such a function call should be kept unchanged.
Agreed. OTOH, we all know that spatch results still need to be carefully
checked. I suspect trying to teach it all the formatting rules of the
kernel isn't an easy task.
> The flag “IORESOURCE_MEM” is passed as the second parameter for the call
> of the function “platform_get_resource” in this refactoring.
In that particular case, we maybe should consider separate inline
helpers instead of passing this is a parameter.
Maybe it would even be more efficient to have completely separate
versions of devm_platform_ioremap_resource(), so we don't even have
to pass that parameter on stack.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]
On Tue, 11 Jun 2019, Enrico Weigelt, metux IT consult wrote:
> On 09.06.19 10:55, Markus Elfring wrote:
>
> <snip>
>
> >> But there is not usually any interesting formatting on the left side of an
> >> assignment (ie typically no newlines or comments).
> >
> > Is there any need to trigger additional source code reformatting?
> >
> >> I can see no purpose to factorizing the right parenthesis.
> >
> > These characters at the end of such a function call should be kept unchanged.
>
> Agreed. OTOH, we all know that spatch results still need to be carefully
> checked. I suspect trying to teach it all the formatting rules of the
> kernel isn't an easy task.
>
> > The flag “IORESOURCE_MEM” is passed as the second parameter for the call
> > of the function “platform_get_resource” in this refactoring.
>
> In that particular case, we maybe should consider separate inline
> helpers instead of passing this is a parameter.
>
> Maybe it would even be more efficient to have completely separate
> versions of devm_platform_ioremap_resource(), so we don't even have
> to pass that parameter on stack.
I'm lost as to why this discussion suddenly appeared. What problem is
actually being discussed?
julia
[-- Attachment #2: Type: text/plain, Size: 136 bytes --]
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

>> The flag “IORESOURCE_MEM” is passed as the second parameter for the call
>> of the function “platform_get_resource” in this refactoring.
>
> In that particular case, we maybe should consider separate inline
> helpers instead of passing this is a parameter.
>
> Maybe it would even be more efficient to have completely separate
> versions of devm_platform_ioremap_resource(), so we don't even have
> to pass that parameter on stack.
Would you like to add another function which should be called instead of
the combination of the functions “platform_get_resource” and “devm_ioremap_resource”?
Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

>> Two function calls were combined in this function implementation.
>> Inline corresponding code so that extra error checks can be avoided here.
>
> I don't see any point to this at all.
Would you like to take another look at corresponding design options?
How do you think about to check run time characteristics any more?
> By inlining the code, you have created a clone,
> which will introduce extra work to maintain in the future.
Would you find the shown software transformation acceptable
if a C compiler will be able to generate a similar code structure?
Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

>> Two function calls were combined in this function implementation.
>> Inline corresponding code so that extra error checks can be avoided here.
>
> What exactly is the purpose of this ?
I suggest to take another look at the need and relevance of involved
error checks in the discussed function combination.
> Looks like a notable code duplication ...
This can be.
> I thought we usually try to reduce this, instead of introducing new ones.
Would you like to check the software circumstances once more
for the generation of a similar code structure by a C compiler
(or optimiser)?
Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

On 18.06.19 07:37, Markus Elfring wrote:
>>> Two function calls were combined in this function implementation.
>>> Inline corresponding code so that extra error checks can be avoided here.
>>
>> What exactly is the purpose of this ?
>
> I suggest to take another look at the need and relevance of involved
> error checks in the discussed function combination.
Sorry, don't have the time for guessing and trying to reproduce your
thoughts. That's why we have patch descriptions / commit messages.
It would be a lot easier for all of us if you just desribe the exact
problem you'd like to solve and your approach to do so.
>> Looks like a notable code duplication ...
>
> This can be.
I doubt that code duplication is appreciated, as this increases the
maintenance overhead. (actually, we're usually trying to reduce that,
eg. by using lots of generic helpers).
>> I thought we usually try to reduce this, instead of introducing new ones.
>
> Would you like to check the software circumstances once more
> for the generation of a similar code structure by a C compiler
> (or optimiser)?
As said: unfortunately, I don't have the time to do that - you'd have to
tell us, what exactly you've got in mind.
If it's just about some error checks which happen to be redundant in a
particular case, you'll have to show that this case is a *really* hot
path (eg. irq, syscall, scheduling, etc) - but I don't see that here.
What's the exact scenario you're trying to optimize ? Any actual
measurements on how your patch improves that ?
Look, I understand that you'd like to squeeze out maximum performance,
but this has to be practically maintainable. I could list a lot of
things that I don't need in particular use cases and would like to
introduce build knobs for, but I have to understand that maintainers
have to be pretty reluctant towards those things.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

>> Would you like to check the software circumstances once more
>> for the generation of a similar code structure by a C compiler
>> (or optimiser)?
>
> As said: unfortunately, I don't have the time to do that
I became curious if you would like to adjust your software development
attention a bit more also in this area.
> - you'd have to tell us, what exactly you've got in mind.
I try to point possibilities out to improve the combination of
two functions.
> If it's just about some error checks which happen to be redundant in a
> particular case, you'll have to show that this case is a *really* hot
> path (eg. irq, syscall, scheduling, etc) - but I don't see that here.
1. May the check “resource_type(res) == IORESOURCE_MEM” be performed
in a local loop?
2. How hot do you find the null pointer check for the device
input parameter of the function “devm_ioremap_resource”?
> Any actual measurements on how your patch improves that ?
Not yet. - Which benchmarks would you trust here?
> Look, I understand that you'd like to squeeze out maximum performance,
I hope so.
> but this has to be practically maintainable.
This can be achieved if more contributors would find proposed
adjustments helpful for another software transformation.
Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci