diff mbox series

checkpatch: fix a false check against wchar/utf-16 string

Message ID 20200616054307.6017-1-takahiro.akashi@linaro.org
State Needs Review / ACK
Delegated to: Tom Rini
Headers show
Series checkpatch: fix a false check against wchar/utf-16 string | expand

Commit Message

AKASHI Takahiro June 16, 2020, 5:43 a.m. UTC
UEFI subsystem uses utf-16 string, but checkpatch.pl complains
about any occurrences of L"xxx" which is definitely legal.
So just suppress this kind of warning.
Precautiously, we will check u"xxx" as well.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Tom Rini June 16, 2020, 2:17 p.m. UTC | #1
On Tue, Jun 16, 2020 at 02:43:07PM +0900, AKASHI Takahiro wrote:

> UEFI subsystem uses utf-16 string, but checkpatch.pl complains
> about any occurrences of L"xxx" which is definitely legal.
> So just suppress this kind of warning.
> Precautiously, we will check u"xxx" as well.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index edba36565167..b3697720787c 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -5462,7 +5462,7 @@ sub process {
>  		}
>  
>  # concatenated string without spaces between elements
> -		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /[A-Za-z0-9_]$String/) {
> +		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /([A-Za-z0-9_]+[Lu]|[A-Za-z0-9_]*[A-KM-Za-tv-z0-9_])$String/) {
>  			if (CHK("CONCATENATED_STRING",
>  				"Concatenated strings should use spaces between elements\n" . $herecurr) &&
>  			    $fix) {

This looks like a generic checkpatch issue.  I think we're a little out
of sync with the kernel's v5.7 but this doesn't look to be fixed there
either.  Can you please submit it upstream?  Thanks!
Heinrich Schuchardt July 2, 2020, 5 p.m. UTC | #2
On 16.06.20 16:17, Tom Rini wrote:
> On Tue, Jun 16, 2020 at 02:43:07PM +0900, AKASHI Takahiro wrote:
>
>> UEFI subsystem uses utf-16 string, but checkpatch.pl complains
>> about any occurrences of L"xxx" which is definitely legal.
>> So just suppress this kind of warning.
>> Precautiously, we will check u"xxx" as well.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>  scripts/checkpatch.pl | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index edba36565167..b3697720787c 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -5462,7 +5462,7 @@ sub process {
>>  		}
>>
>>  # concatenated string without spaces between elements
>> -		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /[A-Za-z0-9_]$String/) {
>> +		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /([A-Za-z0-9_]+[Lu]|[A-Za-z0-9_]*[A-KM-Za-tv-z0-9_])$String/) {
>>  			if (CHK("CONCATENATED_STRING",
>>  				"Concatenated strings should use spaces between elements\n" . $herecurr) &&
>>  			    $fix) {
>
> This looks like a generic checkpatch issue.  I think we're a little out
> of sync with the kernel's v5.7 but this doesn't look to be fixed there
> either.  Can you please submit it upstream?  Thanks!
>

Hello Tom,

I already raised that issue to the Kernel people and they were not
interested in fixing it:

https://lkml.org/lkml/2017/10/17/1086

Recently a lot of changes have been done to the U-Boot version of
checkpatch.pl.

Do we want to diverge from the upstream and have a lot of work each time
we try to sync? Or should we use checkpatch.pl as is and live without
U-Boot specific stuff?

Best regards

Heinrich
Tom Rini July 2, 2020, 8:26 p.m. UTC | #3
On Thu, Jul 02, 2020 at 07:00:04PM +0200, Heinrich Schuchardt wrote:
> On 16.06.20 16:17, Tom Rini wrote:
> > On Tue, Jun 16, 2020 at 02:43:07PM +0900, AKASHI Takahiro wrote:
> >
> >> UEFI subsystem uses utf-16 string, but checkpatch.pl complains
> >> about any occurrences of L"xxx" which is definitely legal.
> >> So just suppress this kind of warning.
> >> Precautiously, we will check u"xxx" as well.
> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>  scripts/checkpatch.pl | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index edba36565167..b3697720787c 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -5462,7 +5462,7 @@ sub process {
> >>  		}
> >>
> >>  # concatenated string without spaces between elements
> >> -		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /[A-Za-z0-9_]$String/) {
> >> +		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /([A-Za-z0-9_]+[Lu]|[A-Za-z0-9_]*[A-KM-Za-tv-z0-9_])$String/) {
> >>  			if (CHK("CONCATENATED_STRING",
> >>  				"Concatenated strings should use spaces between elements\n" . $herecurr) &&
> >>  			    $fix) {
> >
> > This looks like a generic checkpatch issue.  I think we're a little out
> > of sync with the kernel's v5.7 but this doesn't look to be fixed there
> > either.  Can you please submit it upstream?  Thanks!
> >
> 
> Hello Tom,
> 
> I already raised that issue to the Kernel people and they were not
> interested in fixing it:
> 
> https://lkml.org/lkml/2017/10/17/1086
> 
> Recently a lot of changes have been done to the U-Boot version of
> checkpatch.pl.
> 
> Do we want to diverge from the upstream and have a lot of work each time
> we try to sync? Or should we use checkpatch.pl as is and live without
> U-Boot specific stuff?

I would raise the issue with Joe again to see if he still doesn't want
to support wide strings.  If he doesn't I worry that since this change
is outside of the new u-boot function it will get removed by accident
with a future resync.
AKASHI Takahiro July 3, 2020, 5:45 a.m. UTC | #4
On Thu, Jul 02, 2020 at 04:26:34PM -0400, Tom Rini wrote:
> On Thu, Jul 02, 2020 at 07:00:04PM +0200, Heinrich Schuchardt wrote:
> > On 16.06.20 16:17, Tom Rini wrote:
> > > On Tue, Jun 16, 2020 at 02:43:07PM +0900, AKASHI Takahiro wrote:
> > >
> > >> UEFI subsystem uses utf-16 string, but checkpatch.pl complains
> > >> about any occurrences of L"xxx" which is definitely legal.
> > >> So just suppress this kind of warning.
> > >> Precautiously, we will check u"xxx" as well.
> > >>
> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >> ---
> > >>  scripts/checkpatch.pl | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > >> index edba36565167..b3697720787c 100755
> > >> --- a/scripts/checkpatch.pl
> > >> +++ b/scripts/checkpatch.pl
> > >> @@ -5462,7 +5462,7 @@ sub process {
> > >>  		}
> > >>
> > >>  # concatenated string without spaces between elements
> > >> -		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /[A-Za-z0-9_]$String/) {
> > >> +		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /([A-Za-z0-9_]+[Lu]|[A-Za-z0-9_]*[A-KM-Za-tv-z0-9_])$String/) {
> > >>  			if (CHK("CONCATENATED_STRING",
> > >>  				"Concatenated strings should use spaces between elements\n" . $herecurr) &&
> > >>  			    $fix) {
> > >
> > > This looks like a generic checkpatch issue.  I think we're a little out
> > > of sync with the kernel's v5.7 but this doesn't look to be fixed there
> > > either.  Can you please submit it upstream?  Thanks!
> > >
> > 
> > Hello Tom,
> > 
> > I already raised that issue to the Kernel people and they were not
> > interested in fixing it:
> > 
> > https://lkml.org/lkml/2017/10/17/1086
> > 
> > Recently a lot of changes have been done to the U-Boot version of
> > checkpatch.pl.
> > 
> > Do we want to diverge from the upstream and have a lot of work each time
> > we try to sync? Or should we use checkpatch.pl as is and live without
> > U-Boot specific stuff?
> 
> I would raise the issue with Joe again to see if he still doesn't want
> to support wide strings.  If he doesn't I worry that since this change
> is outside of the new u-boot function it will get removed by accident
> with a future resync.

To be strict, Joe's comment, "Kernel doesn't support wide strings."
is no longer true. We can see several examples of L"..." in
    security/integrity/platform_certs/load_uefi.c

-Takahiro Akashi

> -- 
> Tom
Heinrich Schuchardt July 3, 2020, 6:08 a.m. UTC | #5
Am 3. Juli 2020 07:45:03 MESZ schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
>On Thu, Jul 02, 2020 at 04:26:34PM -0400, Tom Rini wrote:
>> On Thu, Jul 02, 2020 at 07:00:04PM +0200, Heinrich Schuchardt wrote:
>> > On 16.06.20 16:17, Tom Rini wrote:
>> > > On Tue, Jun 16, 2020 at 02:43:07PM +0900, AKASHI Takahiro wrote:
>> > >
>> > >> UEFI subsystem uses utf-16 string, but checkpatch.pl complains
>> > >> about any occurrences of L"xxx" which is definitely legal.
>> > >> So just suppress this kind of warning.
>> > >> Precautiously, we will check u"xxx" as well.
>> > >>
>> > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> > >> ---
>> > >>  scripts/checkpatch.pl | 2 +-
>> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >>
>> > >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> > >> index edba36565167..b3697720787c 100755
>> > >> --- a/scripts/checkpatch.pl
>> > >> +++ b/scripts/checkpatch.pl
>> > >> @@ -5462,7 +5462,7 @@ sub process {
>> > >>  		}
>> > >>
>> > >>  # concatenated string without spaces between elements
>> > >> -		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~
>/[A-Za-z0-9_]$String/) {
>> > >> +		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~
>/([A-Za-z0-9_]+[Lu]|[A-Za-z0-9_]*[A-KM-Za-tv-z0-9_])$String/) {
>> > >>  			if (CHK("CONCATENATED_STRING",
>> > >>  				"Concatenated strings should use spaces between elements\n"
>. $herecurr) &&
>> > >>  			    $fix) {
>> > >
>> > > This looks like a generic checkpatch issue.  I think we're a
>little out
>> > > of sync with the kernel's v5.7 but this doesn't look to be fixed
>there
>> > > either.  Can you please submit it upstream?  Thanks!
>> > >
>> > 
>> > Hello Tom,
>> > 
>> > I already raised that issue to the Kernel people and they were not
>> > interested in fixing it:
>> > 
>> > https://lkml.org/lkml/2017/10/17/1086
>> > 
>> > Recently a lot of changes have been done to the U-Boot version of
>> > checkpatch.pl.
>> > 
>> > Do we want to diverge from the upstream and have a lot of work each
>time
>> > we try to sync? Or should we use checkpatch.pl as is and live
>without
>> > U-Boot specific stuff?
>> 
>> I would raise the issue with Joe again to see if he still doesn't
>want
>> to support wide strings.  If he doesn't I worry that since this
>change
>> is outside of the new u-boot function it will get removed by accident
>> with a future resync.
>
>To be strict, Joe's comment, "Kernel doesn't support wide strings."
>is no longer true. We can see several examples of L"..." in
>    security/integrity/platform_certs/load_uefi.c

That makes things easier. Please, check if the upstream checkpatch.pl still has the issue. If yes, please, send your pstch upstream.

Best regards

Heinrich

>
>-Takahiro Akashi
>
>> -- 
>> Tom
diff mbox series

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index edba36565167..b3697720787c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5462,7 +5462,7 @@  sub process {
 		}
 
 # concatenated string without spaces between elements
-		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /[A-Za-z0-9_]$String/) {
+		if ($line =~ /$String[A-Za-z0-9_]/ || $line =~ /([A-Za-z0-9_]+[Lu]|[A-Za-z0-9_]*[A-KM-Za-tv-z0-9_])$String/) {
 			if (CHK("CONCATENATED_STRING",
 				"Concatenated strings should use spaces between elements\n" . $herecurr) &&
 			    $fix) {