diff mbox

checkpatch: allow strings split across lines

Message ID 5530D293.8040204@linaro.org
State New
Headers show

Commit Message

Taras Kondratiuk April 17, 2015, 9:29 a.m. UTC
We can update checkpatch.pl to ignore these cases



On 04/17/2015 09:46 AM, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> I agree with Ola and Taras. Strings should not be broken, otherwise 
> those are hard to find. I think the kernel style has the same 
> recommendation.
> 
> -Petri
> 
> *From:*lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of 
> *ext Ola Liljedahl
> *Sent:* Thursday, April 16, 2015 5:15 PM
> *To:* Mike Holmes
> *Cc:* lng-odp
> *Subject:* Re: [lng-odp] [PATCH] checkpatch: allow strings split across 
> lines
> 
> On 16 April 2015 at 15:50, Mike Holmes <mike.holmes@linaro.org 
> <mailto:mike.holmes@linaro.org>> wrote:
> 
> On 16 April 2015 at 09:41, Taras Kondratiuk <taras.kondratiuk@linaro.org 
> <mailto:taras.kondratiuk@linaro.org>> wrote:
> 
> On 04/16/2015 04:29 PM, Mike Holmes wrote:
> 
> We have an 80 char limit that is frequently an issue for strings and we
> just accept that we ignore the warning.
> 
> Allow split strings so that there is a valid alternative
> Thus the following example becomes a legal alternative to the > 80 chars
> warning.
> 
> printf("\nThread %u (id=%d core=%d) had %u sync_failures"
>          " in %u iterations\n", thread_num,
>          .....
> 
> 
> The reason to have an exception for printed strings length is to have
> them in one line to be searchable in a codebase.
> 
> In this example grep'ing for 'sync_failures in' won't find this string.
> 
> The above format string would better be expressed as
> 
>   printf("\nThread %u (id=%d core=%d) had %u sync_failures in "
> 
>          "%u iterations\n", thread_num,
> 
> E.g. break at a formatting directive.
> 
> But grepping using regular expressions ("had .* sync_failures in .* 
> iterations") would still fail.
> 
> So allowing for lines (with strings) longer than 80 chars is preferable.
> 
> 
>     As with Google you just take a portion of the string if you really
>     have an issue.
> 
>     I rarely include syntactic elements in a search, in the same way
>     that if you search for this string verbatim with he specific thread
>     id, core and number of failures set in the actual string seen on
>     stdout you would not find the code either.
> 
>     -- 
> 
>     Mike Holmes
> 
>     Technical Manager - Linaro Networking Group
> 
>     Linaro.org <http://www.linaro.org/>***│ *Open source software for
>     ARM SoCs
> 
> 
>     _______________________________________________
>     lng-odp mailing list
>     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
>     https://lists.linaro.org/mailman/listinfo/lng-odp
> 
> 
> 
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/lng-odp
>

Comments

Mike Holmes April 17, 2015, 5:52 p.m. UTC | #1
On 17 April 2015 at 05:29, Taras Kondratiuk <taras.kondratiuk@linaro.org>
wrote:

> We can update checkpatch.pl to ignore these cases
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 57d0a24..d90394e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -272,7 +272,9 @@ our $logFunctions = qr{(?x:
>
> [a-z0-9]+_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_rateli
>         WARN(?:_RATELIMIT|_ONCE|)|
>         panic|
> -       MODULE_[A-Z_]+
> +       MODULE_[A-Z_]+|
> +       ODP_ASSERT|ODP_DBG|ODP_ERR|ODP_ABORT|ODP_LOG|ODP_PRINT|
> +       printf
>  )};
>
>  our $signature_tags = qr{(?xi:
>
>
>
This looks promising.

I'd like to take the latest checkpatch from the kernel, it is a little more
specific about some things.
Maxim suggested that since it changes the rules about what is  acceptable
that we take it after 1.1.0 on the 30th which sounds sensible to me.



> On 04/17/2015 09:46 AM, Savolainen, Petri (Nokia - FI/Espoo) wrote:
> > I agree with Ola and Taras. Strings should not be broken, otherwise
> > those are hard to find. I think the kernel style has the same
> > recommendation.
> >
> > -Petri
> >
> > *From:*lng-odp [mailto:lng-odp-bounces@lists.linaro.org] *On Behalf Of
> > *ext Ola Liljedahl
> > *Sent:* Thursday, April 16, 2015 5:15 PM
> > *To:* Mike Holmes
> > *Cc:* lng-odp
> > *Subject:* Re: [lng-odp] [PATCH] checkpatch: allow strings split across
> > lines
> >
> > On 16 April 2015 at 15:50, Mike Holmes <mike.holmes@linaro.org
> > <mailto:mike.holmes@linaro.org>> wrote:
> >
> > On 16 April 2015 at 09:41, Taras Kondratiuk <taras.kondratiuk@linaro.org
> > <mailto:taras.kondratiuk@linaro.org>> wrote:
> >
> > On 04/16/2015 04:29 PM, Mike Holmes wrote:
> >
> > We have an 80 char limit that is frequently an issue for strings and we
> > just accept that we ignore the warning.
> >
> > Allow split strings so that there is a valid alternative
> > Thus the following example becomes a legal alternative to the > 80 chars
> > warning.
> >
> > printf("\nThread %u (id=%d core=%d) had %u sync_failures"
> >          " in %u iterations\n", thread_num,
> >          .....
> >
> >
> > The reason to have an exception for printed strings length is to have
> > them in one line to be searchable in a codebase.
> >
> > In this example grep'ing for 'sync_failures in' won't find this string.
> >
> > The above format string would better be expressed as
> >
> >   printf("\nThread %u (id=%d core=%d) had %u sync_failures in "
> >
> >          "%u iterations\n", thread_num,
> >
> > E.g. break at a formatting directive.
> >
> > But grepping using regular expressions ("had .* sync_failures in .*
> > iterations") would still fail.
> >
> > So allowing for lines (with strings) longer than 80 chars is preferable.
> >
> >
> >     As with Google you just take a portion of the string if you really
> >     have an issue.
> >
> >     I rarely include syntactic elements in a search, in the same way
> >     that if you search for this string verbatim with he specific thread
> >     id, core and number of failures set in the actual string seen on
> >     stdout you would not find the code either.
> >
> >     --
> >
> >     Mike Holmes
> >
> >     Technical Manager - Linaro Networking Group
> >
> >     Linaro.org <http://www.linaro.org/>***│ *Open source software for
> >     ARM SoCs
> >
> >
> >     _______________________________________________
> >     lng-odp mailing list
> >     lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org>
> >     https://lists.linaro.org/mailman/listinfo/lng-odp
> >
> >
> >
> > _______________________________________________
> > lng-odp mailing list
> > lng-odp@lists.linaro.org
> > https://lists.linaro.org/mailman/listinfo/lng-odp
> >
>
>
diff mbox

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 57d0a24..d90394e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -272,7 +272,9 @@  our $logFunctions = qr{(?x:
        [a-z0-9]+_(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_rateli
        WARN(?:_RATELIMIT|_ONCE|)|
        panic|
-       MODULE_[A-Z_]+
+       MODULE_[A-Z_]+|
+       ODP_ASSERT|ODP_DBG|ODP_ERR|ODP_ABORT|ODP_LOG|ODP_PRINT|
+       printf
 )};
 
 our $signature_tags = qr{(?xi: