Patchwork [v2] isdn: avm: Fix string plus integer warning from Clang

login
register
mail settings
Submitter Nathan Chancellor
Date Jan. 10, 2019, 5:41 a.m.
Message ID <20190110054107.6069-1-natechancellor@gmail.com>
Download mbox | patch
Permalink /patch/696363/
State New
Headers show

Comments

Nathan Chancellor - Jan. 10, 2019, 5:41 a.m.
A recent commit in Clang expanded the -Wstring-plus-int warning, showing
some odd behavior in this file.

drivers/isdn/hardware/avm/b1.c:426:30: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
                cinfo->version[j] = "\0\0" + 1;
                                    ~~~~~~~^~~
drivers/isdn/hardware/avm/b1.c:426:30: note: use array indexing to silence this warning
                cinfo->version[j] = "\0\0" + 1;
                                           ^
                                    &      [  ]
1 warning generated.

This is equivalent to just "\0". Nick pointed out that it is smarter to
use "" instead of "\0" because "" is used elsewhere in the kernel and
can be deduplicated at the linking stage.

Link: https://github.com/ClangBuiltLinux/linux/issues/309
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

v1 -> v2:

* Use "" instead of "\0", as they are equivalent, but "" can be
  deduplicated by the linker, as pointed out by Nick.

 drivers/isdn/hardware/avm/b1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Nick Desaulniers - Jan. 10, 2019, 6:57 p.m.
On Wed, Jan 9, 2019 at 9:42 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> A recent commit in Clang expanded the -Wstring-plus-int warning, showing
> some odd behavior in this file.
>
> drivers/isdn/hardware/avm/b1.c:426:30: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
>                 cinfo->version[j] = "\0\0" + 1;
>                                     ~~~~~~~^~~
> drivers/isdn/hardware/avm/b1.c:426:30: note: use array indexing to silence this warning
>                 cinfo->version[j] = "\0\0" + 1;
>                                            ^
>                                     &      [  ]
> 1 warning generated.
>
> This is equivalent to just "\0". Nick pointed out that it is smarter to
> use "" instead of "\0" because "" is used elsewhere in the kernel and
> can be deduplicated at the linking stage.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/309
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>

Thanks for sending the v2.  I'll leave review to the maintainers to
confirm my suspicions about the code's intent.

> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>
> v1 -> v2:
>
> * Use "" instead of "\0", as they are equivalent, but "" can be
>   deduplicated by the linker, as pointed out by Nick.
>
>  drivers/isdn/hardware/avm/b1.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/isdn/hardware/avm/b1.c b/drivers/isdn/hardware/avm/b1.c
> index 4ac378e48902..40ca1e8fa09f 100644
> --- a/drivers/isdn/hardware/avm/b1.c
> +++ b/drivers/isdn/hardware/avm/b1.c
> @@ -423,7 +423,7 @@ void b1_parse_version(avmctrl_info *cinfo)
>         int i, j;
>
>         for (j = 0; j < AVM_MAXVERSION; j++)
> -               cinfo->version[j] = "\0\0" + 1;
> +               cinfo->version[j] = "";
>         for (i = 0, j = 0;
>              j < AVM_MAXVERSION && i < cinfo->versionlen;
>              j++, i += cinfo->versionbuf[i] + 1)
> --
> 2.20.1
>
David Miller - Jan. 19, 2019, 6:02 p.m.
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Wed,  9 Jan 2019 22:41:08 -0700

> A recent commit in Clang expanded the -Wstring-plus-int warning, showing
> some odd behavior in this file.
> 
> drivers/isdn/hardware/avm/b1.c:426:30: warning: adding 'int' to a string does not append to the string [-Wstring-plus-int]
>                 cinfo->version[j] = "\0\0" + 1;
>                                     ~~~~~~~^~~
> drivers/isdn/hardware/avm/b1.c:426:30: note: use array indexing to silence this warning
>                 cinfo->version[j] = "\0\0" + 1;
>                                            ^
>                                     &      [  ]
> 1 warning generated.
> 
> This is equivalent to just "\0". Nick pointed out that it is smarter to
> use "" instead of "\0" because "" is used elsewhere in the kernel and
> can be deduplicated at the linking stage.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/309
> Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 
> v1 -> v2:
> 
> * Use "" instead of "\0", as they are equivalent, but "" can be
>   deduplicated by the linker, as pointed out by Nick.

Applied, that was certainly a funny construct....

It obviously is trying to initialize the array elements to pointers
to empty strings.

Patch

diff --git a/drivers/isdn/hardware/avm/b1.c b/drivers/isdn/hardware/avm/b1.c
index 4ac378e48902..40ca1e8fa09f 100644
--- a/drivers/isdn/hardware/avm/b1.c
+++ b/drivers/isdn/hardware/avm/b1.c
@@ -423,7 +423,7 @@  void b1_parse_version(avmctrl_info *cinfo)
 	int i, j;
 
 	for (j = 0; j < AVM_MAXVERSION; j++)
-		cinfo->version[j] = "\0\0" + 1;
+		cinfo->version[j] = "";
 	for (i = 0, j = 0;
 	     j < AVM_MAXVERSION && i < cinfo->versionlen;
 	     j++, i += cinfo->versionbuf[i] + 1)