| Message ID | 20240911010927.741343-2-lihongbo22@huawei.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 716bf84ef39218a56fadaa413f70da008ad85888 |
| Headers | show |
| Series | coccinelle: Add some rules for string_chioces helpers. | expand |
On Wed, 11 Sep 2024, Hongbo Li wrote: > After str_true_false() has been introduced in the tree, > we can add rules for finding places where str_true_false() > can be used. A simple test can find over 10 locations. > > Signed-off-by: Hongbo Li <lihongbo22@huawei.com> > --- > scripts/coccinelle/api/string_choices.cocci | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci > index 5e729f187f22..6942ad7c4224 100644 > --- a/scripts/coccinelle/api/string_choices.cocci > +++ b/scripts/coccinelle/api/string_choices.cocci > @@ -85,3 +85,22 @@ e << str_down_up_r.E; > @@ > > coccilib.report.print_report(p[0], "opportunity for str_down_up(%s)" % e) > + > +@str_true_false depends on patch@ > +expression E; > +@@ > +- ((E) ? "true" : "false") > ++ str_true_false(E) > + > +@str_true_false_r depends on !patch exists@ > +expression E; > +position P; > +@@ > +* ((E@P) ? "true" : "false") Hello, The semantic patch is quite slow. Actually it tests a large number of cases, eg where the parentheses are present and where they are not. A small optimization is possible in the non-patch case. The outer parentheses are not needed, because you will already get the same information whether they are there or not. In contrast, for the patch case, the outer parentheses are needed, because if they are there we want to remove them, since they are not needed for the function call. Could you update the depends on !patch cases to remove the outer parentheses? Also, just one patch would be fine. There are many changes, but they are all sort of the same, so it would be easier just to see them all at once. thanks, julia > + > +@script:python depends on report@ > +p << str_true_false_r.P; > +e << str_true_false_r.E; > +@@ > + > +coccilib.report.print_report(p[0], "opportunity for str_true_false(%s)" % e) > -- > 2.34.1 > >
> The semantic patch is quite slow. Actually it tests a large number of > cases, eg where the parentheses are present and where they are not. Can such development concerns trigger any adjustments for further coccicheck configurations and provided SmPL scripts? Regards, Markus
On 2024/9/19 14:25, Julia Lawall wrote: > > > On Wed, 11 Sep 2024, Hongbo Li wrote: > >> After str_true_false() has been introduced in the tree, >> we can add rules for finding places where str_true_false() >> can be used. A simple test can find over 10 locations. >> >> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >> --- >> scripts/coccinelle/api/string_choices.cocci | 19 +++++++++++++++++++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci >> index 5e729f187f22..6942ad7c4224 100644 >> --- a/scripts/coccinelle/api/string_choices.cocci >> +++ b/scripts/coccinelle/api/string_choices.cocci >> @@ -85,3 +85,22 @@ e << str_down_up_r.E; >> @@ >> >> coccilib.report.print_report(p[0], "opportunity for str_down_up(%s)" % e) >> + >> +@str_true_false depends on patch@ >> +expression E; >> +@@ >> +- ((E) ? "true" : "false") >> ++ str_true_false(E) >> + >> +@str_true_false_r depends on !patch exists@ >> +expression E; >> +position P; >> +@@ >> +* ((E@P) ? "true" : "false") > > Hello, > > The semantic patch is quite slow. Actually it tests a large number of > cases, eg where the parentheses are present and where they are not. > > A small optimization is possible in the non-patch case. The outer > parentheses are not needed, because you will already get the same > information whether they are there or not. > > In contrast, for the patch case, the outer parentheses are needed, because > if they are there we want to remove them, since they are not needed for > the function call. > > Could you update the depends on !patch cases to remove the outer > parentheses? > You mean in non-patch case, we could just write like the following?: +@str_true_false_r depends on !patch exists@ +expression E; +position P; +@@ +* (E@P) ? "true" : "false" I have tested in my machine. The impact of this parenthesis on performance is very minimal. With parentheses, the time on driver/ costs: real 1m41.696s user 85m24.069s sys 1m8.891s Without parentheses, the time on driver/ costs: real 1m40.438s user 85m53.987s sys 1m7.981s Thanks, Hongbo > Also, just one patch would be fine. There are many changes, but they are > all sort of the same, so it would be easier just to see them all at once. > > thanks, > julia > > >> + >> +@script:python depends on report@ >> +p << str_true_false_r.P; >> +e << str_true_false_r.E; >> +@@ >> + >> +coccilib.report.print_report(p[0], "opportunity for str_true_false(%s)" % e) >> -- >> 2.34.1 >> >>
Thanks for testing it. I will see if there is some other way to improve the performance. Sent from my iPhone > On 23 Sep 2024, at 09:01, Hongbo Li <lihongbo22@huawei.com> wrote: > > > >> On 2024/9/19 14:25, Julia Lawall wrote: >>> On Wed, 11 Sep 2024, Hongbo Li wrote: >>> After str_true_false() has been introduced in the tree, >>> we can add rules for finding places where str_true_false() >>> can be used. A simple test can find over 10 locations. >>> >>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >>> --- >>> scripts/coccinelle/api/string_choices.cocci | 19 +++++++++++++++++++ >>> 1 file changed, 19 insertions(+) >>> >>> diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci >>> index 5e729f187f22..6942ad7c4224 100644 >>> --- a/scripts/coccinelle/api/string_choices.cocci >>> +++ b/scripts/coccinelle/api/string_choices.cocci >>> @@ -85,3 +85,22 @@ e << str_down_up_r.E; >>> @@ >>> >>> coccilib.report.print_report(p[0], "opportunity for str_down_up(%s)" % e) >>> + >>> +@str_true_false depends on patch@ >>> +expression E; >>> +@@ >>> +- ((E) ? "true" : "false") >>> ++ str_true_false(E) >>> + >>> +@str_true_false_r depends on !patch exists@ >>> +expression E; >>> +position P; >>> +@@ >>> +* ((E@P) ? "true" : "false") >> Hello, >> The semantic patch is quite slow. Actually it tests a large number of >> cases, eg where the parentheses are present and where they are not. >> A small optimization is possible in the non-patch case. The outer >> parentheses are not needed, because you will already get the same >> information whether they are there or not. >> In contrast, for the patch case, the outer parentheses are needed, because >> if they are there we want to remove them, since they are not needed for >> the function call. >> Could you update the depends on !patch cases to remove the outer >> parentheses? > > You mean in non-patch case, we could just write like the following?: > > +@str_true_false_r depends on !patch exists@ > +expression E; > +position P; > +@@ > +* (E@P) ? "true" : "false" > > I have tested in my machine. The impact of this parenthesis on performance is very minimal. > > With parentheses, the time on driver/ costs: > > real 1m41.696s > user 85m24.069s > sys 1m8.891s > > Without parentheses, the time on driver/ costs: > > real 1m40.438s > user 85m53.987s > sys 1m7.981s > > > Thanks, > Hongbo > >> Also, just one patch would be fine. There are many changes, but they are >> all sort of the same, so it would be easier just to see them all at once. >> thanks, >> julia >>> + >>> +@script:python depends on report@ >>> +p << str_true_false_r.P; >>> +e << str_true_false_r.E; >>> +@@ >>> + >>> +coccilib.report.print_report(p[0], "opportunity for str_true_false(%s)" % e) >>> -- >>> 2.34.1 >>> >>>
On 2024/9/23 18:24, Julia Lawall wrote: > Thanks for testing it. I will see if there is some other way to improve the performance. May be every rules in the same file are executed sequentially cost a lot? Thanks, Hongbo > Sent from my iPhone > >> On 23 Sep 2024, at 09:01, Hongbo Li <lihongbo22@huawei.com> wrote: >> >> >> >>> On 2024/9/19 14:25, Julia Lawall wrote: >>>> On Wed, 11 Sep 2024, Hongbo Li wrote: >>>> After str_true_false() has been introduced in the tree, >>>> we can add rules for finding places where str_true_false() >>>> can be used. A simple test can find over 10 locations. >>>> >>>> Signed-off-by: Hongbo Li <lihongbo22@huawei.com> >>>> --- >>>> scripts/coccinelle/api/string_choices.cocci | 19 +++++++++++++++++++ >>>> 1 file changed, 19 insertions(+) >>>> >>>> diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci >>>> index 5e729f187f22..6942ad7c4224 100644 >>>> --- a/scripts/coccinelle/api/string_choices.cocci >>>> +++ b/scripts/coccinelle/api/string_choices.cocci >>>> @@ -85,3 +85,22 @@ e << str_down_up_r.E; >>>> @@ >>>> >>>> coccilib.report.print_report(p[0], "opportunity for str_down_up(%s)" % e) >>>> + >>>> +@str_true_false depends on patch@ >>>> +expression E; >>>> +@@ >>>> +- ((E) ? "true" : "false") >>>> ++ str_true_false(E) >>>> + >>>> +@str_true_false_r depends on !patch exists@ >>>> +expression E; >>>> +position P; >>>> +@@ >>>> +* ((E@P) ? "true" : "false") >>> Hello, >>> The semantic patch is quite slow. Actually it tests a large number of >>> cases, eg where the parentheses are present and where they are not. >>> A small optimization is possible in the non-patch case. The outer >>> parentheses are not needed, because you will already get the same >>> information whether they are there or not. >>> In contrast, for the patch case, the outer parentheses are needed, because >>> if they are there we want to remove them, since they are not needed for >>> the function call. >>> Could you update the depends on !patch cases to remove the outer >>> parentheses? >> >> You mean in non-patch case, we could just write like the following?: >> >> +@str_true_false_r depends on !patch exists@ >> +expression E; >> +position P; >> +@@ >> +* (E@P) ? "true" : "false" >> >> I have tested in my machine. The impact of this parenthesis on performance is very minimal. >> >> With parentheses, the time on driver/ costs: >> >> real 1m41.696s >> user 85m24.069s >> sys 1m8.891s >> >> Without parentheses, the time on driver/ costs: >> >> real 1m40.438s >> user 85m53.987s >> sys 1m7.981s >> >> >> Thanks, >> Hongbo >> >>> Also, just one patch would be fine. There are many changes, but they are >>> all sort of the same, so it would be easier just to see them all at once. >>> thanks, >>> julia >>>> + >>>> +@script:python depends on report@ >>>> +p << str_true_false_r.P; >>>> +e << str_true_false_r.E; >>>> +@@ >>>> + >>>> +coccilib.report.print_report(p[0], "opportunity for str_true_false(%s)" % e) >>>> -- >>>> 2.34.1 >>>> >>>> >
diff --git a/scripts/coccinelle/api/string_choices.cocci b/scripts/coccinelle/api/string_choices.cocci index 5e729f187f22..6942ad7c4224 100644 --- a/scripts/coccinelle/api/string_choices.cocci +++ b/scripts/coccinelle/api/string_choices.cocci @@ -85,3 +85,22 @@ e << str_down_up_r.E; @@ coccilib.report.print_report(p[0], "opportunity for str_down_up(%s)" % e) + +@str_true_false depends on patch@ +expression E; +@@ +- ((E) ? "true" : "false") ++ str_true_false(E) + +@str_true_false_r depends on !patch exists@ +expression E; +position P; +@@ +* ((E@P) ? "true" : "false") + +@script:python depends on report@ +p << str_true_false_r.P; +e << str_true_false_r.E; +@@ + +coccilib.report.print_report(p[0], "opportunity for str_true_false(%s)" % e)
After str_true_false() has been introduced in the tree, we can add rules for finding places where str_true_false() can be used. A simple test can find over 10 locations. Signed-off-by: Hongbo Li <lihongbo22@huawei.com> --- scripts/coccinelle/api/string_choices.cocci | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)