| Message ID | 20250128-converge-secs-to-jiffies-part-two-v1-1-9a6ecf0b2308@linux.microsoft.com |
|---|---|
| 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
On 1/30/2025 3:01 AM, Markus Elfring wrote: >> 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)? > Thank you, this is much more useful feedback, specifically due to the suggested patch below. I did intend to learn about the other modes and progressively upgrade secs_to_jiffies.cocci with them in the future once the existing instances were resolved, to help with future code submissions. The patch below will be super helpful in that. As it stands, I'll fix up the current rules in v2 following your suggestion to keep the multiplication in each line to allow Coccinelle to use the commutativity properties and find more instances. I'll refrain from implementing the report mode until current instances have been fixed because of the issue we have already seen[1] with CI builds being broken. I would not want to break a strict CI build that is looking for coccicheck REPORT to return 0 results. [1]: https://lore.kernel.org/all/20250129-secs_to_jiffles-v1-1-35a5e16b9f03@chromium.org/ <snip> Thanks, Easwar (he/him)
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(-)