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

Message ID 20190110054107.6069-1-natechancellor@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [v2] isdn: avm: Fix string plus integer warning from Clang
Related show

Commit Message

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(-)

Comments

Nick Desaulniers Jan. 10, 2019, 6:57 p.m. | #1
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. | #2
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)