* [PATCH] isdn: avm: Fix string plus integer warning from Clang
@ 2019-01-08 5:06 Nathan Chancellor
2019-01-08 17:50 ` Nick Desaulniers
2019-01-10 5:41 ` [PATCH v2] " Nathan Chancellor
0 siblings, 2 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-01-08 5:06 UTC (permalink / raw)
To: Karsten Keil, David S. Miller
Cc: netdev, linux-kernel, Nick Desaulniers, Nathan Chancellor
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" so fix it to clean up the warning.
Link: https://github.com/ClangBuiltLinux/linux/issues/309
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
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..7fa8141e2019 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] = "\0";
for (i = 0, j = 0;
j < AVM_MAXVERSION && i < cinfo->versionlen;
j++, i += cinfo->versionbuf[i] + 1)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] isdn: avm: Fix string plus integer warning from Clang
2019-01-08 5:06 [PATCH] isdn: avm: Fix string plus integer warning from Clang Nathan Chancellor
@ 2019-01-08 17:50 ` Nick Desaulniers
2019-01-08 17:54 ` Nathan Chancellor
2019-01-10 5:41 ` [PATCH v2] " Nathan Chancellor
1 sibling, 1 reply; 6+ messages in thread
From: Nick Desaulniers @ 2019-01-08 17:50 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: Karsten Keil, David S. Miller, netdev, LKML
On Mon, Jan 7, 2019 at 9:07 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" so fix it to clean up the warning.
Hard to tell what the intent of this was. I also assume that they
meant to set all version strings to the empty string. I think just `=
""` would be better, as otherwise that creates a new data section
containing two zero bytes vs the empty string which already probably
exists throughout the kernel and can be deduplicated by the linker.
https://godbolt.org/z/qf3Rn8
Sorry I gave the quick LGTM in
https://github.com/ClangBuiltLinux/linux/issues/309#issuecomment-452106468,
that was my mistake. Assuming there's no other comments (maybe from
the maintainers) on what the intent of this code actually is, would
you mind sending a V2? (Sorry again for the quick LGTM on this
version).
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/309
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> 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..7fa8141e2019 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] = "\0";
> for (i = 0, j = 0;
> j < AVM_MAXVERSION && i < cinfo->versionlen;
> j++, i += cinfo->versionbuf[i] + 1)
> --
> 2.20.1
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] isdn: avm: Fix string plus integer warning from Clang
2019-01-08 17:50 ` Nick Desaulniers
@ 2019-01-08 17:54 ` Nathan Chancellor
0 siblings, 0 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-01-08 17:54 UTC (permalink / raw)
To: Nick Desaulniers; +Cc: Karsten Keil, David S. Miller, netdev, LKML
On Tue, Jan 08, 2019 at 09:50:39AM -0800, Nick Desaulniers wrote:
> On Mon, Jan 7, 2019 at 9:07 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" so fix it to clean up the warning.
>
> Hard to tell what the intent of this was. I also assume that they
> meant to set all version strings to the empty string. I think just `=
> ""` would be better, as otherwise that creates a new data section
> containing two zero bytes vs the empty string which already probably
> exists throughout the kernel and can be deduplicated by the linker.
> https://godbolt.org/z/qf3Rn8
> Sorry I gave the quick LGTM in
> https://github.com/ClangBuiltLinux/linux/issues/309#issuecomment-452106468,
> that was my mistake. Assuming there's no other comments (maybe from
> the maintainers) on what the intent of this code actually is, would
> you mind sending a V2? (Sorry again for the quick LGTM on this
> version).
Of course, thank you for the explanation and further exploration. I'll
send a v2 later today/tomorrow to give some time for the maintainers to
chime in.
>
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/309
> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > ---
> > 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..7fa8141e2019 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] = "\0";
> > for (i = 0, j = 0;
> > j < AVM_MAXVERSION && i < cinfo->versionlen;
> > j++, i += cinfo->versionbuf[i] + 1)
> > --
> > 2.20.1
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] isdn: avm: Fix string plus integer warning from Clang
2019-01-08 5:06 [PATCH] isdn: avm: Fix string plus integer warning from Clang Nathan Chancellor
2019-01-08 17:50 ` Nick Desaulniers
@ 2019-01-10 5:41 ` Nathan Chancellor
2019-01-10 18:57 ` Nick Desaulniers
2019-01-19 18:02 ` David Miller
1 sibling, 2 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-01-10 5:41 UTC (permalink / raw)
To: Karsten Keil, David S. Miller
Cc: netdev, linux-kernel, Nathan Chancellor, Nick Desaulniers
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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] isdn: avm: Fix string plus integer warning from Clang
2019-01-10 5:41 ` [PATCH v2] " Nathan Chancellor
@ 2019-01-10 18:57 ` Nick Desaulniers
2019-01-19 18:02 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: Nick Desaulniers @ 2019-01-10 18:57 UTC (permalink / raw)
To: Nathan Chancellor; +Cc: Karsten Keil, David S. Miller, netdev, LKML
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
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] isdn: avm: Fix string plus integer warning from Clang
2019-01-10 5:41 ` [PATCH v2] " Nathan Chancellor
2019-01-10 18:57 ` Nick Desaulniers
@ 2019-01-19 18:02 ` David Miller
1 sibling, 0 replies; 6+ messages in thread
From: David Miller @ 2019-01-19 18:02 UTC (permalink / raw)
To: natechancellor; +Cc: isdn, netdev, linux-kernel, ndesaulniers
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-19 18:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-08 5:06 [PATCH] isdn: avm: Fix string plus integer warning from Clang Nathan Chancellor
2019-01-08 17:50 ` Nick Desaulniers
2019-01-08 17:54 ` Nathan Chancellor
2019-01-10 5:41 ` [PATCH v2] " Nathan Chancellor
2019-01-10 18:57 ` Nick Desaulniers
2019-01-19 18:02 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).