| Message ID | 20250128-converge-secs-to-jiffies-part-two-v1-1-9a6ecf0b2308@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Converge on using secs_to_jiffies() part two | expand |
> Teach the script to suggest conversions for timeout patterns where the > arguments to msecs_to_jiffies() are expressions as well. I propose to take another look at implementation details for such a script variant according to the semantic patch language. … > +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci > @@ -11,12 +11,22 @@ > > virtual patch … > -@depends on patch@ constant C; @@ > +@depends on patch@ > +expression E; > +@@ > > -- msecs_to_jiffies(C * MSEC_PER_SEC) > -+ secs_to_jiffies(C) > +-msecs_to_jiffies > ++secs_to_jiffies > + (E > +- * \( 1000 \| MSEC_PER_SEC \) > + ) 1. I do not see a need to keep an SmPL rule for the handling of constants (or literals) after the suggested extension for expressions. 2. I find it nice that you indicate an attempt to make the shown SmPL code a bit more succinct. Unfortunately, further constraints should be taken better into account for the current handling of isomorphisms (and corresponding SmPL disjunctions). Thus I would find an SmPL rule (like the following) more appropriate. @adjustment@ expression e; @@ -msecs_to_jiffies +secs_to_jiffies ( ( -e * 1000 | -e * MSEC_PER_SEC ) +e ) 3. It seems that you would like to support only a single operation mode so far. This system aspect can trigger further software development challenges. Regards, Markus
On 1/28/2025 1:02 PM, Markus Elfring wrote: >> Teach the script to suggest conversions for timeout patterns where the >> arguments to msecs_to_jiffies() are expressions as well. > > I propose to take another look at implementation details for such a script variant > according to the semantic patch language. > > > … >> +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci >> @@ -11,12 +11,22 @@ >> >> virtual patch > … >> -@depends on patch@ constant C; @@ >> +@depends on patch@ >> +expression E; >> +@@ >> >> -- msecs_to_jiffies(C * MSEC_PER_SEC) >> -+ secs_to_jiffies(C) >> +-msecs_to_jiffies >> ++secs_to_jiffies >> + (E >> +- * \( 1000 \| MSEC_PER_SEC \) >> + ) > > 1. I do not see a need to keep an SmPL rule for the handling of constants > (or literals) after the suggested extension for expressions. Can you explain why? Would the expression rule also address the cases where it's a constant or literal? > 2. I find it nice that you indicate an attempt to make the shown SmPL code > a bit more succinct. > Unfortunately, further constraints should be taken better into account > for the current handling of isomorphisms (and corresponding SmPL disjunctions). > Thus I would find an SmPL rule (like the following) more appropriate. > Sorry, I couldn't follow your sentence construction or reasoning here. I don't see how my patch is deficient, or different from your suggestion below, especially given that it follows your feedback from part 1: https://lore.kernel.org/all/9088f9a2-c4ab-4098-a255-25120df5c497@web.de/ Can you point out specifically what SmPL isomorphisms or disjunctions are broken with the patch in its current state? Thanks, Easwar
>> … >>> +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci >>> @@ -11,12 +11,22 @@ >>> >>> virtual patch >> … >>> -@depends on patch@ constant C; @@ >>> +@depends on patch@ >>> +expression E; >>> +@@ >>> >>> -- msecs_to_jiffies(C * MSEC_PER_SEC) >>> -+ secs_to_jiffies(C) >>> +-msecs_to_jiffies >>> ++secs_to_jiffies >>> + (E >>> +- * \( 1000 \| MSEC_PER_SEC \) >>> + ) >> >> 1. I do not see a need to keep an SmPL rule for the handling of constants >> (or literals) after the suggested extension for expressions. > > Can you explain why? Would the expression rule also address the cases > where it's a constant or literal? Probably, yes. >> 2. I find it nice that you indicate an attempt to make the shown SmPL code >> a bit more succinct. >> Unfortunately, further constraints should be taken better into account >> for the current handling of isomorphisms (and corresponding SmPL disjunctions). >> Thus I would find an SmPL rule (like the following) more appropriate. >> > > Sorry, I couldn't follow your sentence construction or reasoning here. > I don't see how my patch is deficient, or different from your suggestion > below, especially given that it follows your feedback from part 1: > https://lore.kernel.org/all/9088f9a2-c4ab-4098-a255-25120df5c497@web.de/ I tend also to present possibilities for succinct SmPL code. Unfortunately, software dependencies can trigger corresponding target conflicts. > Can you point out specifically what SmPL isomorphisms or disjunctions > are broken with the patch in its current state? Please take another look at related information sources. Would you like to achieve any benefits from commutativity (for multiplications)? https://gitlab.inria.fr/coccinelle/coccinelle/-/blob/bd08cad3f802229dc629a13eefef2018c620e905/standard.iso#L241 https://github.com/coccinelle/coccinelle/blob/cca22217d1b4316224e80a18d0b08dd351234497/standard.iso#L241 Regards, Markus
> Teach the script to suggest conversions for timeout patterns where the > arguments to msecs_to_jiffies() are expressions as well. Does anything hinder to benefit any more from a source code analysis approach (like the following by the extended means of the semantic patch language)? // SPDX-License-Identifier: GPL-2.0 /// Simplify statements by using a known wrapper macro. /// Replace selected msecs_to_jiffies() calls by secs_to_jiffies(). // // Keywords: wrapper macro conversion secs seconds jiffies // Confidence: High // Options: --no-includes --include-headers virtual context, patch, report, org @depends on context@ expression e; @@ *msecs_to_jiffies ( (e * 1000 |e * MSEC_PER_SEC ) ) @depends on patch@ expression e; @@ -msecs_to_jiffies +secs_to_jiffies ( ( -e * 1000 | -e * MSEC_PER_SEC ) +e ) @x depends on org || report@ expression e; position p; @@ msecs_to_jiffies@p ( (e * 1000 |e * MSEC_PER_SEC ) ) @script:python depends on org@ p << x.p; @@ coccilib.org.print_todo(p[0], "WARNING: opportunity for secs_to_jiffies()") @script:python depends on report@ p << x.p; @@ coccilib.report.print_report(p[0], "WARNING: opportunity for secs_to_jiffies()") Regards, Markus
diff --git a/scripts/coccinelle/misc/secs_to_jiffies.cocci b/scripts/coccinelle/misc/secs_to_jiffies.cocci index 8bbb2884ea5db939c63fd4513cf5ca8c977aa8cb..ab9880d239f7d2a8d56a481a2710369e1082e16e 100644 --- a/scripts/coccinelle/misc/secs_to_jiffies.cocci +++ b/scripts/coccinelle/misc/secs_to_jiffies.cocci @@ -11,12 +11,22 @@ virtual patch -@depends on patch@ constant C; @@ +@depends on patch@ +constant C; +@@ -- msecs_to_jiffies(C * 1000) -+ secs_to_jiffies(C) +-msecs_to_jiffies ++secs_to_jiffies + (C +- * \( 1000 \| MSEC_PER_SEC \) + ) -@depends on patch@ constant C; @@ +@depends on patch@ +expression E; +@@ -- msecs_to_jiffies(C * MSEC_PER_SEC) -+ secs_to_jiffies(C) +-msecs_to_jiffies ++secs_to_jiffies + (E +- * \( 1000 \| MSEC_PER_SEC \) + )
Teach the script to suggest conversions for timeout patterns where the arguments to msecs_to_jiffies() are expressions as well. Signed-off-by: Easwar Hariharan <eahariha@linux.microsoft.com> --- scripts/coccinelle/misc/secs_to_jiffies.cocci | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-)