* [PATCH 0/4] Get rid of some more warnings given by clang @ 2017-06-21 20:15 Simon Marchi 2017-06-21 20:15 ` [PATCH 2/4] x86-dregs: Print debug registers one per line Simon Marchi ` (3 more replies) 0 siblings, 4 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-21 20:15 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi This is another small series that gets rid of some warnings given by clang. Simon Marchi (4): environ-selftests: Ignore -Wself-move warning x86-dregs: Print debug registers one per line dtrace-probe: Put semicolon after while on its own line main: Don't add int to string gdb/dtrace-probe.c | 3 ++- gdb/main.c | 2 +- gdb/nat/x86-dregs.c | 16 ++++++---------- gdb/unittests/environ-selftests.c | 9 +++++++++ 4 files changed, 18 insertions(+), 12 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 2/4] x86-dregs: Print debug registers one per line 2017-06-21 20:15 [PATCH 0/4] Get rid of some more warnings given by clang Simon Marchi @ 2017-06-21 20:15 ` Simon Marchi 2017-06-21 20:31 ` Sergio Durigan Junior 2017-06-21 20:15 ` [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line Simon Marchi ` (2 subsequent siblings) 3 siblings, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-21 20:15 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi This get around this warning given by clang... /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: variable 'i' is incremented both in the loop header and in the loop body [-Werror,-Wfor-loop-analysis] i++; ^ /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: incremented here ALL_DEBUG_ADDRESS_REGISTERS (i) ^ ... I decided in the end to simply print the debug registers one per line. I don't think it particularly helps readability to have them two per line anyway. gdb/ChangeLog: * nat/x86-dregs.c (x86_show_dr): Print registers one per line. --- gdb/nat/x86-dregs.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c index 8c8adfa..58b1179 100644 --- a/gdb/nat/x86-dregs.c +++ b/gdb/nat/x86-dregs.c @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state, here. */ : "??unknown??")))); debug_printf (":\n"); - debug_printf ("\tCONTROL (DR7): %s STATUS (DR6): %s\n", - phex (state->dr_control_mirror, 8), - phex (state->dr_status_mirror, 8)); + + debug_printf ("\tCONTROL (DR7): 0x%s\n",phex (state->dr_control_mirror, 8)); + debug_printf ("\tSTATUS (DR6): 0x%s\n", phex (state->dr_status_mirror, 8)); + ALL_DEBUG_ADDRESS_REGISTERS (i) { - debug_printf ("\ -\tDR%d: addr=0x%s, ref.count=%d DR%d: addr=0x%s, ref.count=%d\n", + debug_printf ("\tDR%d: addr=0x%s, ref.count=%d\n", i, phex (state->dr_mirror[i], x86_get_debug_register_length ()), - state->dr_ref_count[i], - i + 1, phex (state->dr_mirror[i + 1], - x86_get_debug_register_length ()), - state->dr_ref_count[i + 1]); - i++; + state->dr_ref_count[i]); } } -- 2.7.4 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/4] x86-dregs: Print debug registers one per line 2017-06-21 20:15 ` [PATCH 2/4] x86-dregs: Print debug registers one per line Simon Marchi @ 2017-06-21 20:31 ` Sergio Durigan Junior 2017-06-21 21:06 ` Simon Marchi 0 siblings, 1 reply; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 20:31 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On Wednesday, June 21 2017, Simon Marchi wrote: > This get around this warning given by clang... > > /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: variable 'i' is incremented both in the loop header and in the loop body [-Werror,-Wfor-loop-analysis] > i++; > ^ > /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: incremented here > ALL_DEBUG_ADDRESS_REGISTERS (i) > ^ > > ... I decided in the end to simply print the debug registers one per > line. I don't think it particularly helps readability to have them two > per line anyway. Agreed, one per line sounds better to me. > > gdb/ChangeLog: > > * nat/x86-dregs.c (x86_show_dr): Print registers one per line. > --- > gdb/nat/x86-dregs.c | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c > index 8c8adfa..58b1179 100644 > --- a/gdb/nat/x86-dregs.c > +++ b/gdb/nat/x86-dregs.c > @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state, > here. */ > : "??unknown??")))); > debug_printf (":\n"); > - debug_printf ("\tCONTROL (DR7): %s STATUS (DR6): %s\n", > - phex (state->dr_control_mirror, 8), > - phex (state->dr_status_mirror, 8)); > + > + debug_printf ("\tCONTROL (DR7): 0x%s\n",phex (state->dr_control_mirror, 8)); ^^^ Space after comma? > + debug_printf ("\tSTATUS (DR6): 0x%s\n", phex (state->dr_status_mirror, 8)); > + > ALL_DEBUG_ADDRESS_REGISTERS (i) > { > - debug_printf ("\ > -\tDR%d: addr=0x%s, ref.count=%d DR%d: addr=0x%s, ref.count=%d\n", > + debug_printf ("\tDR%d: addr=0x%s, ref.count=%d\n", > i, phex (state->dr_mirror[i], > x86_get_debug_register_length ()), > - state->dr_ref_count[i], > - i + 1, phex (state->dr_mirror[i + 1], > - x86_get_debug_register_length ()), > - state->dr_ref_count[i + 1]); > - i++; > + state->dr_ref_count[i]); > } > } > > -- > 2.7.4 LGTM. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/4] x86-dregs: Print debug registers one per line 2017-06-21 20:31 ` Sergio Durigan Junior @ 2017-06-21 21:06 ` Simon Marchi 2017-06-25 10:58 ` Simon Marchi 0 siblings, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-21 21:06 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 2017-06-21 22:31, Sergio Durigan Junior wrote: > On Wednesday, June 21 2017, Simon Marchi wrote: > >> This get around this warning given by clang... >> >> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: >> variable 'i' is incremented both in the loop header and in the loop >> body [-Werror,-Wfor-loop-analysis] >> i++; >> ^ >> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: >> incremented here >> ALL_DEBUG_ADDRESS_REGISTERS (i) >> ^ >> >> ... I decided in the end to simply print the debug registers one per >> line. I don't think it particularly helps readability to have them >> two >> per line anyway. > > Agreed, one per line sounds better to me. > >> >> gdb/ChangeLog: >> >> * nat/x86-dregs.c (x86_show_dr): Print registers one per line. >> --- >> gdb/nat/x86-dregs.c | 16 ++++++---------- >> 1 file changed, 6 insertions(+), 10 deletions(-) >> >> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c >> index 8c8adfa..58b1179 100644 >> --- a/gdb/nat/x86-dregs.c >> +++ b/gdb/nat/x86-dregs.c >> @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state, >> here. */ >> : "??unknown??")))); >> debug_printf (":\n"); >> - debug_printf ("\tCONTROL (DR7): %s STATUS (DR6): %s\n", >> - phex (state->dr_control_mirror, 8), >> - phex (state->dr_status_mirror, 8)); >> + >> + debug_printf ("\tCONTROL (DR7): 0x%s\n",phex >> (state->dr_control_mirror, 8)); > ^^^ > > Space after comma? Yep, thanks. >> + debug_printf ("\tSTATUS (DR6): 0x%s\n", phex >> (state->dr_status_mirror, 8)); >> + >> ALL_DEBUG_ADDRESS_REGISTERS (i) >> { >> - debug_printf ("\ >> -\tDR%d: addr=0x%s, ref.count=%d DR%d: addr=0x%s, ref.count=%d\n", >> + debug_printf ("\tDR%d: addr=0x%s, ref.count=%d\n", >> i, phex (state->dr_mirror[i], >> x86_get_debug_register_length ()), >> - state->dr_ref_count[i], >> - i + 1, phex (state->dr_mirror[i + 1], >> - x86_get_debug_register_length ()), >> - state->dr_ref_count[i + 1]); >> - i++; >> + state->dr_ref_count[i]); >> } >> } >> >> -- >> 2.7.4 > > LGTM. Thanks, Thanks! ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 2/4] x86-dregs: Print debug registers one per line 2017-06-21 21:06 ` Simon Marchi @ 2017-06-25 10:58 ` Simon Marchi 0 siblings, 0 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-25 10:58 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 2017-06-21 23:06, Simon Marchi wrote: > On 2017-06-21 22:31, Sergio Durigan Junior wrote: >> On Wednesday, June 21 2017, Simon Marchi wrote: >> >>> This get around this warning given by clang... >>> >>> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:209:7: error: >>> variable 'i' is incremented both in the loop header and in the loop >>> body [-Werror,-Wfor-loop-analysis] >>> i++; >>> ^ >>> /home/emaisin/src/binutils-gdb/gdb/nat/x86-dregs.c:199:32: note: >>> incremented here >>> ALL_DEBUG_ADDRESS_REGISTERS (i) >>> ^ >>> >>> ... I decided in the end to simply print the debug registers one per >>> line. I don't think it particularly helps readability to have them >>> two >>> per line anyway. >> >> Agreed, one per line sounds better to me. >> >>> >>> gdb/ChangeLog: >>> >>> * nat/x86-dregs.c (x86_show_dr): Print registers one per line. >>> --- >>> gdb/nat/x86-dregs.c | 16 ++++++---------- >>> 1 file changed, 6 insertions(+), 10 deletions(-) >>> >>> diff --git a/gdb/nat/x86-dregs.c b/gdb/nat/x86-dregs.c >>> index 8c8adfa..58b1179 100644 >>> --- a/gdb/nat/x86-dregs.c >>> +++ b/gdb/nat/x86-dregs.c >>> @@ -193,20 +193,16 @@ x86_show_dr (struct x86_debug_reg_state *state, >>> here. */ >>> : "??unknown??")))); >>> debug_printf (":\n"); >>> - debug_printf ("\tCONTROL (DR7): %s STATUS (DR6): %s\n", >>> - phex (state->dr_control_mirror, 8), >>> - phex (state->dr_status_mirror, 8)); >>> + >>> + debug_printf ("\tCONTROL (DR7): 0x%s\n",phex >>> (state->dr_control_mirror, 8)); >> ^^^ >> >> Space after comma? > > Yep, thanks. > >>> + debug_printf ("\tSTATUS (DR6): 0x%s\n", phex >>> (state->dr_status_mirror, 8)); >>> + >>> ALL_DEBUG_ADDRESS_REGISTERS (i) >>> { >>> - debug_printf ("\ >>> -\tDR%d: addr=0x%s, ref.count=%d DR%d: addr=0x%s, ref.count=%d\n", >>> + debug_printf ("\tDR%d: addr=0x%s, ref.count=%d\n", >>> i, phex (state->dr_mirror[i], >>> x86_get_debug_register_length ()), >>> - state->dr_ref_count[i], >>> - i + 1, phex (state->dr_mirror[i + 1], >>> - x86_get_debug_register_length ()), >>> - state->dr_ref_count[i + 1]); >>> - i++; >>> + state->dr_ref_count[i]); >>> } >>> } >>> >>> -- >>> 2.7.4 >> >> LGTM. Thanks, > > Thanks! This is now pushed. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line 2017-06-21 20:15 [PATCH 0/4] Get rid of some more warnings given by clang Simon Marchi 2017-06-21 20:15 ` [PATCH 2/4] x86-dregs: Print debug registers one per line Simon Marchi @ 2017-06-21 20:15 ` Simon Marchi 2017-06-21 20:34 ` Sergio Durigan Junior 2017-06-21 21:36 ` Pedro Alves 2017-06-21 20:15 ` [PATCH 4/4] main: Don't add int to string Simon Marchi 2017-06-21 20:15 ` [PATCH 1/4] environ-selftests: Ignore -Wself-move warning Simon Marchi 3 siblings, 2 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-21 20:15 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi clang shows this warning. /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: while loop has empty body [-Werror,-Wempty-body] while (*p++ != '\0' && p - strtab < strtab_size); ^ /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put the semicolon on a separate line to silence this warning Putting the semicolon on its own line is not a big sacrifice to get rid of this warning. I think it's also useful to keep this, because it can catch errors like this: while (something); { ... } although gcc would warn about it in a different way (misleading indentation). This warning is already discussed here in the GCC bugzilla: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184 gdb/ChangeLog: * dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on its own line. --- gdb/dtrace-probe.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c index 122f8de..9a02694 100644 --- a/gdb/dtrace-probe.c +++ b/gdb/dtrace-probe.c @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile, arg.type_str = xstrdup (p); /* Use strtab_size as a sentinel. */ - while (*p++ != '\0' && p - strtab < strtab_size); + while (*p++ != '\0' && p - strtab < strtab_size) + ; /* Silence clang's -Wempty-body warning. */ /* Try to parse a type expression from the type string. If this does not work then we set the type to `long -- 2.7.4 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line 2017-06-21 20:15 ` [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line Simon Marchi @ 2017-06-21 20:34 ` Sergio Durigan Junior 2017-06-21 21:08 ` Simon Marchi 2017-06-21 21:36 ` Pedro Alves 1 sibling, 1 reply; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 20:34 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On Wednesday, June 21 2017, Simon Marchi wrote: > clang shows this warning. > > /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: while loop has empty body [-Werror,-Wempty-body] > while (*p++ != '\0' && p - strtab < strtab_size); > ^ > /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put the semicolon on a separate line to silence this warning > > Putting the semicolon on its own line is not a big sacrifice to get rid of this > warning. I think it's also useful to keep this, because it can catch errors > like this: > > while (something); > { > ... > } > > although gcc would warn about it in a different way (misleading indentation). > > This warning is already discussed here in the GCC bugzilla: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184 > > gdb/ChangeLog: > > * dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on > its own line. > --- > gdb/dtrace-probe.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c > index 122f8de..9a02694 100644 > --- a/gdb/dtrace-probe.c > +++ b/gdb/dtrace-probe.c > @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile, > arg.type_str = xstrdup (p); > > /* Use strtab_size as a sentinel. */ > - while (*p++ != '\0' && p - strtab < strtab_size); > + while (*p++ != '\0' && p - strtab < strtab_size) > + ; /* Silence clang's -Wempty-body warning. */ Lately I've been choosing to explicitly put "continue;" when the body doesn't contain anything, like: while (*p++ != '\0' && p - strtab < strtab_size) continue; I don't know what others think about it, but it would solve this problem and also be more verbose that we're just iterating without a body. > > /* Try to parse a type expression from the type string. If > this does not work then we set the type to `long > -- > 2.7.4 -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line 2017-06-21 20:34 ` Sergio Durigan Junior @ 2017-06-21 21:08 ` Simon Marchi 0 siblings, 0 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-21 21:08 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 2017-06-21 22:34, Sergio Durigan Junior wrote: > On Wednesday, June 21 2017, Simon Marchi wrote: > >> clang shows this warning. >> >> /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: error: >> while loop has empty body [-Werror,-Wempty-body] >> while (*p++ != '\0' && p - strtab < strtab_size); >> ^ >> /home/emaisin/src/binutils-gdb/gdb/dtrace-probe.c:424:52: note: put >> the semicolon on a separate line to silence this warning >> >> Putting the semicolon on its own line is not a big sacrifice to get >> rid of this >> warning. I think it's also useful to keep this, because it can catch >> errors >> like this: >> >> while (something); >> { >> ... >> } >> >> although gcc would warn about it in a different way (misleading >> indentation). >> >> This warning is already discussed here in the GCC bugzilla: >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=62184 >> >> gdb/ChangeLog: >> >> * dtrace-probe.c (dtrace_process_dof_probe): Put semi-colon on >> its own line. >> --- >> gdb/dtrace-probe.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/gdb/dtrace-probe.c b/gdb/dtrace-probe.c >> index 122f8de..9a02694 100644 >> --- a/gdb/dtrace-probe.c >> +++ b/gdb/dtrace-probe.c >> @@ -421,7 +421,8 @@ dtrace_process_dof_probe (struct objfile *objfile, >> arg.type_str = xstrdup (p); >> >> /* Use strtab_size as a sentinel. */ >> - while (*p++ != '\0' && p - strtab < strtab_size); >> + while (*p++ != '\0' && p - strtab < strtab_size) >> + ; /* Silence clang's -Wempty-body warning. */ > > Lately I've been choosing to explicitly put "continue;" when the body > doesn't contain anything, like: > > while (*p++ != '\0' && p - strtab < strtab_size) > continue; > > I don't know what others think about it, but it would solve this > problem > and also be more verbose that we're just iterating without a body. I also looks good, but I don't have a preference. I'll do that if others like it too. Thanks, Simon ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line 2017-06-21 20:15 ` [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line Simon Marchi 2017-06-21 20:34 ` Sergio Durigan Junior @ 2017-06-21 21:36 ` Pedro Alves 2017-06-25 10:48 ` Simon Marchi 1 sibling, 1 reply; 29+ messages in thread From: Pedro Alves @ 2017-06-21 21:36 UTC (permalink / raw) To: Simon Marchi, gdb-patches On 06/21/2017 09:15 PM, Simon Marchi wrote: > /* Use strtab_size as a sentinel. */ > - while (*p++ != '\0' && p - strtab < strtab_size); > + while (*p++ != '\0' && p - strtab < strtab_size) > + ; /* Silence clang's -Wempty-body warning. */ I'd must put the ; on its own line (there's probably something in the coding conventions about this already), and without the comment. It's quite common to write for/while loop like that, see e.g.,: $ grep "^[ |\t]*;[ |\t]*$" *.c -C 2 Sure the comment makes sense in the context of the patch, but when reading the code without considering the patch's context, it just looks like noise to me. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line 2017-06-21 21:36 ` Pedro Alves @ 2017-06-25 10:48 ` Simon Marchi 2017-06-25 10:57 ` Simon Marchi 0 siblings, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-25 10:48 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches On 2017-06-21 23:36, Pedro Alves wrote: > On 06/21/2017 09:15 PM, Simon Marchi wrote: >> /* Use strtab_size as a sentinel. */ >> - while (*p++ != '\0' && p - strtab < strtab_size); >> + while (*p++ != '\0' && p - strtab < strtab_size) >> + ; /* Silence clang's -Wempty-body warning. */ > > I'd must put the ; on its own line (there's probably something > in the coding conventions about this already), and without the > comment. It's quite common to write for/while loop like that, > see e.g.,: > > $ grep "^[ |\t]*;[ |\t]*$" *.c -C 2 > > Sure the comment makes sense in the context of the patch, > but when reading the code without considering the patch's context, > it just looks like noise to me. Ok, since that's already a common practice in our codebase, I added it to our wiki (I didn't find anything related to that in the GNU standards): https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#preview I am pushing without the comment. Thanks, Simon ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line 2017-06-25 10:48 ` Simon Marchi @ 2017-06-25 10:57 ` Simon Marchi 0 siblings, 0 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-25 10:57 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches On 2017-06-25 12:48, Simon Marchi wrote: > Ok, since that's already a common practice in our codebase, I added it > to our wiki (I didn't find anything related to that in the GNU > standards): > > https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#preview Sorry, partly wrong URL: https://sourceware.org/gdb/wiki/Internals%20GDB-C-Coding-Standards#Loop_with_empty_body ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 4/4] main: Don't add int to string 2017-06-21 20:15 [PATCH 0/4] Get rid of some more warnings given by clang Simon Marchi 2017-06-21 20:15 ` [PATCH 2/4] x86-dregs: Print debug registers one per line Simon Marchi 2017-06-21 20:15 ` [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line Simon Marchi @ 2017-06-21 20:15 ` Simon Marchi 2017-06-21 20:35 ` Sergio Durigan Junior 2017-06-21 20:15 ` [PATCH 1/4] environ-selftests: Ignore -Wself-move warning Simon Marchi 3 siblings, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-21 20:15 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi clang shows this warning: /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: error: adding 'int' to a string does not append to the string [-Werror,-Wstring-plus-int] char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len); ~~~~~~~~~~~~~~~^~~~~~~~~~~~~ /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: note: use array indexing to silence this warning char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len); ^ & [ ] It's quite easy to get rid of it by using &foo[len] instead of foo + len. I think this warning is relevant to keep enabled, because it can be an easy mistake to do. This warning is already discussed here in GCC bugzilla: https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00729.html and a patch series for it was submitted very recently. gdb/ChangeLog: * main.c (get_init_files): Replace "SYSTEM_GDBINIT + datadir_len" with "&SYSTEM_GDBINIT[datadir_len]". --- gdb/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gdb/main.c b/gdb/main.c index df4b111..9813041 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -224,7 +224,7 @@ get_init_files (const char **system_gdbinit, { /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR to gdb_datadir. */ - char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len); + char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]); char *p; for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p) -- 2.7.4 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/4] main: Don't add int to string 2017-06-21 20:15 ` [PATCH 4/4] main: Don't add int to string Simon Marchi @ 2017-06-21 20:35 ` Sergio Durigan Junior 2017-06-25 10:58 ` Simon Marchi 0 siblings, 1 reply; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 20:35 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On Wednesday, June 21 2017, Simon Marchi wrote: > clang shows this warning: > > /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: error: adding 'int' to a string does not append to the string [-Werror,-Wstring-plus-int] > char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len); > ~~~~~~~~~~~~~~~^~~~~~~~~~~~~ > /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: note: use array indexing to silence this warning > char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len); > ^ > & [ ] > > It's quite easy to get rid of it by using &foo[len] instead of foo + len. > I think this warning is relevant to keep enabled, because it can be an > easy mistake to do. > > This warning is already discussed here in GCC bugzilla: > > https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00729.html > > and a patch series for it was submitted very recently. > > gdb/ChangeLog: > > * main.c (get_init_files): Replace "SYSTEM_GDBINIT + > datadir_len" with "&SYSTEM_GDBINIT[datadir_len]". > --- > gdb/main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/main.c b/gdb/main.c > index df4b111..9813041 100644 > --- a/gdb/main.c > +++ b/gdb/main.c > @@ -224,7 +224,7 @@ get_init_files (const char **system_gdbinit, > { > /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR > to gdb_datadir. */ > - char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + datadir_len); > + char *tmp_sys_gdbinit = xstrdup (&SYSTEM_GDBINIT[datadir_len]); > char *p; > > for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p) > -- > 2.7.4 LGTM. Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 4/4] main: Don't add int to string 2017-06-21 20:35 ` Sergio Durigan Junior @ 2017-06-25 10:58 ` Simon Marchi 0 siblings, 0 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-25 10:58 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 2017-06-21 22:35, Sergio Durigan Junior wrote: > On Wednesday, June 21 2017, Simon Marchi wrote: > >> clang shows this warning: >> >> /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: error: adding >> 'int' to a string does not append to the string >> [-Werror,-Wstring-plus-int] >> char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + >> datadir_len); >> >> ~~~~~~~~~~~~~~~^~~~~~~~~~~~~ >> /home/emaisin/src/binutils-gdb/gdb/main.c:227:56: note: use array >> indexing to silence this warning >> char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + >> datadir_len); >> ^ >> & [ >> ] >> >> It's quite easy to get rid of it by using &foo[len] instead of foo + >> len. >> I think this warning is relevant to keep enabled, because it can be an >> easy mistake to do. >> >> This warning is already discussed here in GCC bugzilla: >> >> https://gcc.gnu.org/ml/gcc-patches/2017-06/msg00729.html >> >> and a patch series for it was submitted very recently. >> >> gdb/ChangeLog: >> >> * main.c (get_init_files): Replace "SYSTEM_GDBINIT + >> datadir_len" with "&SYSTEM_GDBINIT[datadir_len]". >> --- >> gdb/main.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gdb/main.c b/gdb/main.c >> index df4b111..9813041 100644 >> --- a/gdb/main.c >> +++ b/gdb/main.c >> @@ -224,7 +224,7 @@ get_init_files (const char **system_gdbinit, >> { >> /* Append the part of SYSTEM_GDBINIT that follows GDB_DATADIR >> to gdb_datadir. */ >> - char *tmp_sys_gdbinit = xstrdup (SYSTEM_GDBINIT + >> datadir_len); >> + char *tmp_sys_gdbinit = xstrdup >> (&SYSTEM_GDBINIT[datadir_len]); >> char *p; >> >> for (p = tmp_sys_gdbinit; IS_DIR_SEPARATOR (*p); ++p) >> -- >> 2.7.4 > > LGTM. > > Thanks, Thanks, pushed. ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 20:15 [PATCH 0/4] Get rid of some more warnings given by clang Simon Marchi ` (2 preceding siblings ...) 2017-06-21 20:15 ` [PATCH 4/4] main: Don't add int to string Simon Marchi @ 2017-06-21 20:15 ` Simon Marchi 2017-06-21 20:29 ` Sergio Durigan Junior 2017-06-22 8:31 ` [PATCH v2] " Simon Marchi 3 siblings, 2 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-21 20:15 UTC (permalink / raw) To: gdb-patches; +Cc: Simon Marchi clang gives this warning: /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: error: explicitly moving variable of type 'gdb_environ' to itself [-Werror,-Wself-move] env = std::move (env); ~~~ ^ ~~~ In this case, ignoring the warning locally is clearly the thing to do, since it warns exactly about the behavior we want to test. We also don't want to disable this globally, because we would want the compiler if we wrote that in real code. I filed a bug in GCC's bugzilla to suggest to add this warning: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159 gdb/ChangeLog: * unittests/environ-selftests.c (run_tests): Ignore -Wself-move warning. --- gdb/unittests/environ-selftests.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c index ecc3955..6989c5e 100644 --- a/gdb/unittests/environ-selftests.c +++ b/gdb/unittests/environ-selftests.c @@ -136,7 +136,16 @@ run_tests () env.clear (); env.set ("A", "1"); SELF_CHECK (strcmp (env.get ("A"), "1") == 0); + +#ifdef __clang__ +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wself-move" +#endif /* __clang__ */ env = std::move (env); +#ifdef __clang__ +#pragma clang diagnostic pop +#endif /* __clang__ */ + SELF_CHECK (strcmp (env.get ("A"), "1") == 0); SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0); SELF_CHECK (env.envp ()[1] == NULL); -- 2.7.4 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 20:15 ` [PATCH 1/4] environ-selftests: Ignore -Wself-move warning Simon Marchi @ 2017-06-21 20:29 ` Sergio Durigan Junior 2017-06-21 21:05 ` Simon Marchi 2017-06-21 21:16 ` Simon Marchi 2017-06-22 8:31 ` [PATCH v2] " Simon Marchi 1 sibling, 2 replies; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 20:29 UTC (permalink / raw) To: Simon Marchi; +Cc: gdb-patches On Wednesday, June 21 2017, Simon Marchi wrote: > clang gives this warning: > > /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: error: explicitly moving variable of type 'gdb_environ' to itself [-Werror,-Wself-move] > env = std::move (env); > ~~~ ^ ~~~ > > In this case, ignoring the warning locally is clearly the thing to do, > since it warns exactly about the behavior we want to test. We also > don't want to disable this globally, because we would want the compiler "we would want the code compiler to warn" > if we wrote that in real code. > > I filed a bug in GCC's bugzilla to suggest to add this warning: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159 Thanks! > gdb/ChangeLog: > > * unittests/environ-selftests.c (run_tests): Ignore -Wself-move > warning. > --- > gdb/unittests/environ-selftests.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c > index ecc3955..6989c5e 100644 > --- a/gdb/unittests/environ-selftests.c > +++ b/gdb/unittests/environ-selftests.c > @@ -136,7 +136,16 @@ run_tests () > env.clear (); > env.set ("A", "1"); > SELF_CHECK (strcmp (env.get ("A"), "1") == 0); > + > +#ifdef __clang__ > +#pragma clang diagnostic push > +#pragma clang diagnostic ignored "-Wself-move" > +#endif /* __clang__ */ > env = std::move (env); > +#ifdef __clang__ > +#pragma clang diagnostic pop > +#endif /* __clang__ */ Wow. I know we've discussed this before, but this is ugly :-/. Anyway, this file is just a unittest, so I'm totally fine with this. Do you think it's worth putting a comment on top, just to explicitly say what this is doing? Otherwise, LGTM. > + > SELF_CHECK (strcmp (env.get ("A"), "1") == 0); > SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0); > SELF_CHECK (env.envp ()[1] == NULL); > -- > 2.7.4 Thanks, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 20:29 ` Sergio Durigan Junior @ 2017-06-21 21:05 ` Simon Marchi 2017-06-21 21:12 ` Sergio Durigan Junior 2017-06-21 21:28 ` Pedro Alves 2017-06-21 21:16 ` Simon Marchi 1 sibling, 2 replies; 29+ messages in thread From: Simon Marchi @ 2017-06-21 21:05 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 2017-06-21 22:29, Sergio Durigan Junior wrote: > On Wednesday, June 21 2017, Simon Marchi wrote: > >> clang gives this warning: >> >> /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: >> error: explicitly moving variable of type 'gdb_environ' to itself >> [-Werror,-Wself-move] >> env = std::move (env); >> ~~~ ^ ~~~ >> >> In this case, ignoring the warning locally is clearly the thing to do, >> since it warns exactly about the behavior we want to test. We also >> don't want to disable this globally, because we would want the >> compiler > > "we would want the code compiler to warn" Oops thanks. >> if we wrote that in real code. >> >> I filed a bug in GCC's bugzilla to suggest to add this warning: >> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159 > > Thanks! > >> gdb/ChangeLog: >> >> * unittests/environ-selftests.c (run_tests): Ignore -Wself-move >> warning. >> --- >> gdb/unittests/environ-selftests.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/gdb/unittests/environ-selftests.c >> b/gdb/unittests/environ-selftests.c >> index ecc3955..6989c5e 100644 >> --- a/gdb/unittests/environ-selftests.c >> +++ b/gdb/unittests/environ-selftests.c >> @@ -136,7 +136,16 @@ run_tests () >> env.clear (); >> env.set ("A", "1"); >> SELF_CHECK (strcmp (env.get ("A"), "1") == 0); >> + >> +#ifdef __clang__ >> +#pragma clang diagnostic push >> +#pragma clang diagnostic ignored "-Wself-move" >> +#endif /* __clang__ */ >> env = std::move (env); >> +#ifdef __clang__ >> +#pragma clang diagnostic pop >> +#endif /* __clang__ */ > > Wow. I know we've discussed this before, but this is ugly :-/. > Anyway, > this file is just a unittest, so I'm totally fine with this. Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. Without them, gcc emits a warning [-Wunknown-pragma]. We always have the option to turn -Wunknown-pragma off globally, what do you prefer? ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 21:05 ` Simon Marchi @ 2017-06-21 21:12 ` Sergio Durigan Junior 2017-06-21 21:28 ` Pedro Alves 1 sibling, 0 replies; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 21:12 UTC (permalink / raw) To: Simon Marchi; +Cc: Simon Marchi, gdb-patches On Wednesday, June 21 2017, Simon Marchi wrote: >>> gdb/ChangeLog: >>> >>> * unittests/environ-selftests.c (run_tests): Ignore -Wself-move >>> warning. >>> --- >>> gdb/unittests/environ-selftests.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/gdb/unittests/environ-selftests.c >>> b/gdb/unittests/environ-selftests.c >>> index ecc3955..6989c5e 100644 >>> --- a/gdb/unittests/environ-selftests.c >>> +++ b/gdb/unittests/environ-selftests.c >>> @@ -136,7 +136,16 @@ run_tests () >>> env.clear (); >>> env.set ("A", "1"); >>> SELF_CHECK (strcmp (env.get ("A"), "1") == 0); >>> + >>> +#ifdef __clang__ >>> +#pragma clang diagnostic push >>> +#pragma clang diagnostic ignored "-Wself-move" >>> +#endif /* __clang__ */ >>> env = std::move (env); >>> +#ifdef __clang__ >>> +#pragma clang diagnostic pop >>> +#endif /* __clang__ */ >> >> Wow. I know we've discussed this before, but this is ugly :-/. >> Anyway, >> this file is just a unittest, so I'm totally fine with this. > > Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. > Without them, gcc emits a warning [-Wunknown-pragma]. We always have > the option to turn -Wunknown-pragma off globally, what do you prefer? I'd prefer to leave the ifdef. It's just a small part in the "ugliness". -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 21:05 ` Simon Marchi 2017-06-21 21:12 ` Sergio Durigan Junior @ 2017-06-21 21:28 ` Pedro Alves 2017-06-21 21:32 ` Sergio Durigan Junior 2017-06-22 7:44 ` Simon Marchi 1 sibling, 2 replies; 29+ messages in thread From: Pedro Alves @ 2017-06-21 21:28 UTC (permalink / raw) To: Simon Marchi, Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 06/21/2017 10:05 PM, Simon Marchi wrote: > Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. > Without them, gcc emits a warning [-Wunknown-pragma]. We always have > the option to turn -Wunknown-pragma off globally, what do you prefer? > Don't both GCC and Clang understand "#pragma GCC diagnostic" instead? Or better even, wrap it in some macros (and use _Pragma): #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") #define DIAGNOSTIC_IGNORE(option) \ _Pragma (STRINGIFY (GCC diagnostic ignored option)) Alternatively, you could replace the std::move with a cast to rvalue ref, which is just what std::move really is: -env = std::move (env); +env = static_cast<gdb_environ &&> (env); Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 21:28 ` Pedro Alves @ 2017-06-21 21:32 ` Sergio Durigan Junior 2017-06-22 7:44 ` Simon Marchi 1 sibling, 0 replies; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 21:32 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, Simon Marchi, gdb-patches On Wednesday, June 21 2017, Pedro Alves wrote: > On 06/21/2017 10:05 PM, Simon Marchi wrote: > >> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. >> Without them, gcc emits a warning [-Wunknown-pragma]. We always have >> the option to turn -Wunknown-pragma off globally, what do you prefer? >> > > Don't both GCC and Clang understand "#pragma GCC diagnostic" instead? > > Or better even, wrap it in some macros (and use _Pragma): > > #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") > #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") > #define DIAGNOSTIC_IGNORE(option) \ > _Pragma (STRINGIFY (GCC diagnostic ignored option)) > > > Alternatively, you could replace the std::move with a cast > to rvalue ref, which is just what std::move really is: > > -env = std::move (env); > +env = static_cast<gdb_environ &&> (env); I don't like this cast TBH. std::move is more readable. -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 21:28 ` Pedro Alves 2017-06-21 21:32 ` Sergio Durigan Junior @ 2017-06-22 7:44 ` Simon Marchi 2017-06-22 9:34 ` Pedro Alves 1 sibling, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-22 7:44 UTC (permalink / raw) To: Pedro Alves; +Cc: Sergio Durigan Junior, Simon Marchi, gdb-patches On 2017-06-21 23:28, Pedro Alves wrote: > On 06/21/2017 10:05 PM, Simon Marchi wrote: > >> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. >> Without them, gcc emits a warning [-Wunknown-pragma]. We always have >> the option to turn -Wunknown-pragma off globally, what do you prefer? >> > > Don't both GCC and Clang understand "#pragma GCC diagnostic" instead? Yes, but then it's GCC that complains that it doesn't know the warning: /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:141:32: error: unknown option after â#pragma GCC diagnosticâ kind [-Werror=pragmas] #pragma GCC diagnostic ignored "-Wself-move" ^ > Or better even, wrap it in some macros (and use _Pragma): > > #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") > #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") > #define DIAGNOSTIC_IGNORE(option) \ > _Pragma (STRINGIFY (GCC diagnostic ignored option)) Oh that's interesting. The gcc doc said that _Pragma was added exactly for this purpose (to be usable in macros). I'll try that. > Alternatively, you could replace the std::move with a cast > to rvalue ref, which is just what std::move really is: > > -env = std::move (env); > +env = static_cast<gdb_environ &&> (env); I guess that with a comment explaining why we use that it would be ok, but that would not be my first choice either. Thanks, Simon ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-22 7:44 ` Simon Marchi @ 2017-06-22 9:34 ` Pedro Alves 0 siblings, 0 replies; 29+ messages in thread From: Pedro Alves @ 2017-06-22 9:34 UTC (permalink / raw) To: Simon Marchi; +Cc: Sergio Durigan Junior, Simon Marchi, gdb-patches On 06/22/2017 08:44 AM, Simon Marchi wrote: > On 2017-06-21 23:28, Pedro Alves wrote: >> On 06/21/2017 10:05 PM, Simon Marchi wrote: >> >>> Yeah, I didn't expect to have to put the #ifdefs for __clang__ though. >>> Without them, gcc emits a warning [-Wunknown-pragma]. We always have >>> the option to turn -Wunknown-pragma off globally, what do you prefer? >>> >> >> Don't both GCC and Clang understand "#pragma GCC diagnostic" instead? > > Yes, but then it's GCC that complains that it doesn't know the warning: > > /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:141:32: > error: unknown option after â#pragma GCC diagnosticâ kind [-Werror=pragmas] > #pragma GCC diagnostic ignored "-Wself-move" > ^ > Ah, but that's a different kind of complaint. Since both compilers understand "#pragma GCC" but only one understands "#pragma clang", then it seems clearly better to me to write the infrastructure macros in terms of the former. It'll make it a bit easier to reuse the macros for other warnings. >> Or better even, wrap it in some macros (and use _Pragma): >> >> #define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") >> #define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") >> #define DIAGNOSTIC_IGNORE(option) \ >> _Pragma (STRINGIFY (GCC diagnostic ignored option)) > > Oh that's interesting. The gcc doc said that _Pragma was added exactly > for this purpose (to be usable in macros). I'll try that. Right. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 20:29 ` Sergio Durigan Junior 2017-06-21 21:05 ` Simon Marchi @ 2017-06-21 21:16 ` Simon Marchi 2017-06-21 21:30 ` Sergio Durigan Junior 1 sibling, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-21 21:16 UTC (permalink / raw) To: Sergio Durigan Junior; +Cc: Simon Marchi, gdb-patches On 2017-06-21 22:29, Sergio Durigan Junior wrote: >> +#ifdef __clang__ >> +#pragma clang diagnostic push >> +#pragma clang diagnostic ignored "-Wself-move" >> +#endif /* __clang__ */ >> env = std::move (env); >> +#ifdef __clang__ >> +#pragma clang diagnostic pop >> +#endif /* __clang__ */ > Do you > think it's worth putting a comment on top, just to explicitly say what > this is doing? Forgot to reply to this. It seemed self-explanatory enough to me that ignoring the "self-move" warning just above a blatant self move was probably to silence a warning pointing out the self move :). ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 1/4] environ-selftests: Ignore -Wself-move warning 2017-06-21 21:16 ` Simon Marchi @ 2017-06-21 21:30 ` Sergio Durigan Junior 0 siblings, 0 replies; 29+ messages in thread From: Sergio Durigan Junior @ 2017-06-21 21:30 UTC (permalink / raw) To: Simon Marchi; +Cc: Simon Marchi, gdb-patches On Wednesday, June 21 2017, Simon Marchi wrote: > On 2017-06-21 22:29, Sergio Durigan Junior wrote: >>> +#ifdef __clang__ >>> +#pragma clang diagnostic push >>> +#pragma clang diagnostic ignored "-Wself-move" >>> +#endif /* __clang__ */ >>> env = std::move (env); >>> +#ifdef __clang__ >>> +#pragma clang diagnostic pop >>> +#endif /* __clang__ */ > >> Do you >> think it's worth putting a comment on top, just to explicitly say what >> this is doing? > > Forgot to reply to this. It seemed self-explanatory enough to me that > ignoring the "self-move" warning just above a blatant self move was > probably to silence a warning pointing out the self move :). Yeah, you're right. I just thought a comment in English would make it *even* clearer ;-). But that's just a personal preference, I don't mind. Cheers, -- Sergio GPG key ID: 237A 54B1 0287 28BF 00EF 31F4 D0EB 7628 65FC 5E36 Please send encrypted e-mail if possible http://sergiodj.net/ ^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2] environ-selftests: Ignore -Wself-move warning 2017-06-21 20:15 ` [PATCH 1/4] environ-selftests: Ignore -Wself-move warning Simon Marchi 2017-06-21 20:29 ` Sergio Durigan Junior @ 2017-06-22 8:31 ` Simon Marchi 2017-06-22 9:51 ` Pedro Alves 1 sibling, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-22 8:31 UTC (permalink / raw) To: gdb-patches; +Cc: sergiodj, palves, Simon Marchi New in v2: add wrapper macros in common/diagnostics.h, use them in environ-selftests.c. clang gives this warning: /home/emaisin/src/binutils-gdb/gdb/unittests/environ-selftests.c:139:7: error: explicitly moving variable of type 'gdb_environ' to itself [-Werror,-Wself-move] env = std::move (env); ~~~ ^ ~~~ In this case, ignoring the warning locally is clearly the thing to do, since it warns exactly about the behavior we want to test. We also don't want to disable this globally, because we would want the compiler to warn if we wrote that in real code. I added the file common/diagnostics.h, in which we can put macros used to control compiler diagnostics. This makes the resulting code less cluttered: #ifdef __clang__ #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wself-move" #endif /* __clang__ */ env = std::move (env); #ifdef __clang__ #pragma clang diagnostic pop #endif /* __clang__ */ vs DIAG_PUSH_IGNORE_SELF_MOVE env = std::move (env); DIAG_POP_IGNORE_SELF_MOVE I also added a comment in the end, as per Sergio's suggestion. It's clear to see that the warning it turned off without it, but I think it's good to have it to justify why it is. I filed a bug in GCC's bugzilla to suggest to add this warning: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81159 gdb/ChangeLog: * unittests/environ-selftests.c (run_tests): Ignore -Wself-move warning. * common/diagnostics.h: New file. --- gdb/common/diagnostics.h | 35 +++++++++++++++++++++++++++++++++++ gdb/unittests/environ-selftests.c | 7 +++++++ 2 files changed, 42 insertions(+) create mode 100644 gdb/common/diagnostics.h diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h new file mode 100644 index 0000000..2bf462c --- /dev/null +++ b/gdb/common/diagnostics.h @@ -0,0 +1,35 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef COMMON_DIAGNOSTICS_H +#define COMMON_DIAGNOSTICS_H + +#if defined (__clang__) + +#define DIAG_PUSH_IGNORE_SELF_MOVE \ + _Pragma("clang diagnostic push") \ + _Pragma("clang diagnostic ignored \"-Wself-move\"") +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop") + +#else + +#define DIAG_PUSH_IGNORE_SELF_MOVE +#define DIAG_POP_IGNORE_SELF_MOVE + +#endif + +#endif /* COMMON_DIAGNOSTICS_H */ diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c index ecc3955..fae9bd0 100644 --- a/gdb/unittests/environ-selftests.c +++ b/gdb/unittests/environ-selftests.c @@ -20,6 +20,7 @@ #include "defs.h" #include "selftest.h" #include "common/environ.h" +#include "common/diagnostics.h" namespace selftests { namespace gdb_environ_tests { @@ -136,7 +137,13 @@ run_tests () env.clear (); env.set ("A", "1"); SELF_CHECK (strcmp (env.get ("A"), "1") == 0); + + /* Some compilers warn about moving to self, but that's precisely what we want + to test here, so turn this warning off. */ + DIAG_PUSH_IGNORE_SELF_MOVE env = std::move (env); + DIAG_POP_IGNORE_SELF_MOVE + SELF_CHECK (strcmp (env.get ("A"), "1") == 0); SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0); SELF_CHECK (env.envp ()[1] == NULL); -- 2.7.4 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning 2017-06-22 8:31 ` [PATCH v2] " Simon Marchi @ 2017-06-22 9:51 ` Pedro Alves 2017-06-22 10:52 ` Pedro Alves 0 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2017-06-22 9:51 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: sergiodj On 06/22/2017 09:31 AM, Simon Marchi wrote: > +#if defined (__clang__) > + > +#define DIAG_PUSH_IGNORE_SELF_MOVE \ > + _Pragma("clang diagnostic push") \ > + _Pragma("clang diagnostic ignored \"-Wself-move\"") > +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop") > + > +#else > + > +#define DIAG_PUSH_IGNORE_SELF_MOVE > +#define DIAG_POP_IGNORE_SELF_MOVE > + > +#endif Let me try writing a quick patch that puts a "STRINGIZE" macro in common/preprocessor.h, so we can write the above like I had suggested before, which is a bit more generic. There are several copies in the tree of such a macro, so that'll be a good thing on its own anyway, IMO. I'm not sure whether the single push-ignore macro is a good idea, since if we follow the pattern going forward, it requires more boilerplace if we need different warnings around the same code: { DIAG_PUSH_IGNORE_WARN1 DIAG_PUSH_IGNORE_WARN2 DIAG_PUSH_IGNORE_WARN3 ... // some code DIAG_POP_IGNORE_WARN3 DIAG_POP_IGNORE_WARN2 DIAG_POP_IGNORE_WARN1 } vs: { DIAG_PUSH DIAG_IGNORE_WARN1 DIAG_IGNORE_WARN2 DIAG_IGNORE_WARN3 ... // some code DIAG_POP } Though we can always support both styles. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning 2017-06-22 9:51 ` Pedro Alves @ 2017-06-22 10:52 ` Pedro Alves 2017-06-22 10:57 ` Simon Marchi 0 siblings, 1 reply; 29+ messages in thread From: Pedro Alves @ 2017-06-22 10:52 UTC (permalink / raw) To: Simon Marchi, gdb-patches; +Cc: sergiodj On 06/22/2017 10:51 AM, Pedro Alves wrote: > On 06/22/2017 09:31 AM, Simon Marchi wrote: >> +#if defined (__clang__) >> + >> +#define DIAG_PUSH_IGNORE_SELF_MOVE \ >> + _Pragma("clang diagnostic push") \ >> + _Pragma("clang diagnostic ignored \"-Wself-move\"") >> +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop") >> + >> +#else >> + >> +#define DIAG_PUSH_IGNORE_SELF_MOVE >> +#define DIAG_POP_IGNORE_SELF_MOVE >> + >> +#endif > > > Let me try writing a quick patch that puts a "STRINGIZE" macro > in common/preprocessor.h, so we can write the above like I had > suggested before, which is a bit more generic. There are several > copies in the tree of such a macro, so that'll be a good thing > on its own anyway, IMO. > > I'm not sure whether the single push-ignore macro is a good idea, > since if we follow the pattern going forward, it requires more > boilerplace if we need different warnings around the same code: This is what I had in mind. I think it should work with clang, though I haven't tried it. Subject: [PATCH] environ-selftests: Ignore -Wself-move warning --- gdb/common/diagnostics.h | 40 +++++++++++++++++++++++++++++++++++++++ gdb/unittests/environ-selftests.c | 8 ++++++++ 2 files changed, 48 insertions(+) create mode 100644 gdb/common/diagnostics.h diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h new file mode 100644 index 0000000..5a63bfd --- /dev/null +++ b/gdb/common/diagnostics.h @@ -0,0 +1,40 @@ +/* Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#ifndef COMMON_DIAGNOSTICS_H +#define COMMON_DIAGNOSTICS_H + +#include "common/preprocessor.h" + +#ifdef __GNUC__ +# define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") +# define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") +# define DIAGNOSTIC_IGNORE(option) \ + _Pragma (STRINGIFY (GCC diagnostic ignored option)) +#else +# define DIAGNOSTIC_PUSH +# define DIAGNOSTIC_POP +# define DIAGNOSTIC_IGNORE(option) +#endif + +#ifdef __clang__ +# define DIAGNOSTIC_IGNORE_SELF_MOVE DIAGNOSTIC_IGNORE ("-Wself-move") +#else +# define DIAGNOSTIC_IGNORE_SELF_MOVE +#endif + +#endif /* COMMON_DIAGNOSTICS_H */ diff --git a/gdb/unittests/environ-selftests.c b/gdb/unittests/environ-selftests.c index ecc3955..28b16f8 100644 --- a/gdb/unittests/environ-selftests.c +++ b/gdb/unittests/environ-selftests.c @@ -20,6 +20,7 @@ #include "defs.h" #include "selftest.h" #include "common/environ.h" +#include "common/diagnostics.h" namespace selftests { namespace gdb_environ_tests { @@ -136,7 +137,14 @@ run_tests () env.clear (); env.set ("A", "1"); SELF_CHECK (strcmp (env.get ("A"), "1") == 0); + + /* Some compilers warn about moving to self, but that's precisely what we want + to test here, so turn this warning off. */ + DIAGNOSTIC_PUSH + DIAGNOSTIC_IGNORE_SELF_MOVE env = std::move (env); + DIAGNOSTIC_POP + SELF_CHECK (strcmp (env.get ("A"), "1") == 0); SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0); SELF_CHECK (env.envp ()[1] == NULL); -- 2.5.5 ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning 2017-06-22 10:52 ` Pedro Alves @ 2017-06-22 10:57 ` Simon Marchi 2017-06-22 11:43 ` Pedro Alves 0 siblings, 1 reply; 29+ messages in thread From: Simon Marchi @ 2017-06-22 10:57 UTC (permalink / raw) To: Pedro Alves; +Cc: Simon Marchi, gdb-patches, sergiodj On 2017-06-22 12:52, Pedro Alves wrote: > On 06/22/2017 10:51 AM, Pedro Alves wrote: >> On 06/22/2017 09:31 AM, Simon Marchi wrote: >>> +#if defined (__clang__) >>> + >>> +#define DIAG_PUSH_IGNORE_SELF_MOVE \ >>> + _Pragma("clang diagnostic push") \ >>> + _Pragma("clang diagnostic ignored \"-Wself-move\"") >>> +#define DIAG_POP_IGNORE_SELF_MOVE _Pragma("clang diagnostic pop") >>> + >>> +#else >>> + >>> +#define DIAG_PUSH_IGNORE_SELF_MOVE >>> +#define DIAG_POP_IGNORE_SELF_MOVE >>> + >>> +#endif >> >> >> Let me try writing a quick patch that puts a "STRINGIZE" macro >> in common/preprocessor.h, so we can write the above like I had >> suggested before, which is a bit more generic. There are several >> copies in the tree of such a macro, so that'll be a good thing >> on its own anyway, IMO. >> >> I'm not sure whether the single push-ignore macro is a good idea, >> since if we follow the pattern going forward, it requires more >> boilerplace if we need different warnings around the same code: > > This is what I had in mind. I think it should work with clang, > though I haven't tried it. > > > Subject: [PATCH] environ-selftests: Ignore -Wself-move warning > > --- > gdb/common/diagnostics.h | 40 > +++++++++++++++++++++++++++++++++++++++ > gdb/unittests/environ-selftests.c | 8 ++++++++ > 2 files changed, 48 insertions(+) > create mode 100644 gdb/common/diagnostics.h > > diff --git a/gdb/common/diagnostics.h b/gdb/common/diagnostics.h > new file mode 100644 > index 0000000..5a63bfd > --- /dev/null > +++ b/gdb/common/diagnostics.h > @@ -0,0 +1,40 @@ > +/* Copyright (C) 2017 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or > modify > + it under the terms of the GNU General Public License as published > by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see > <http://www.gnu.org/licenses/>. */ > + > +#ifndef COMMON_DIAGNOSTICS_H > +#define COMMON_DIAGNOSTICS_H > + > +#include "common/preprocessor.h" > + > +#ifdef __GNUC__ > +# define DIAGNOSTIC_PUSH _Pragma ("GCC diagnostic push") > +# define DIAGNOSTIC_POP _Pragma ("GCC diagnostic pop") > +# define DIAGNOSTIC_IGNORE(option) \ > + _Pragma (STRINGIFY (GCC diagnostic ignored option)) > +#else > +# define DIAGNOSTIC_PUSH > +# define DIAGNOSTIC_POP > +# define DIAGNOSTIC_IGNORE(option) > +#endif > + > +#ifdef __clang__ > +# define DIAGNOSTIC_IGNORE_SELF_MOVE DIAGNOSTIC_IGNORE ("-Wself-move") > +#else > +# define DIAGNOSTIC_IGNORE_SELF_MOVE > +#endif > + > +#endif /* COMMON_DIAGNOSTICS_H */ > diff --git a/gdb/unittests/environ-selftests.c > b/gdb/unittests/environ-selftests.c > index ecc3955..28b16f8 100644 > --- a/gdb/unittests/environ-selftests.c > +++ b/gdb/unittests/environ-selftests.c > @@ -20,6 +20,7 @@ > #include "defs.h" > #include "selftest.h" > #include "common/environ.h" > +#include "common/diagnostics.h" > > namespace selftests { > namespace gdb_environ_tests { > @@ -136,7 +137,14 @@ run_tests () > env.clear (); > env.set ("A", "1"); > SELF_CHECK (strcmp (env.get ("A"), "1") == 0); > + > + /* Some compilers warn about moving to self, but that's precisely > what we want > + to test here, so turn this warning off. */ > + DIAGNOSTIC_PUSH > + DIAGNOSTIC_IGNORE_SELF_MOVE > env = std::move (env); > + DIAGNOSTIC_POP > + > SELF_CHECK (strcmp (env.get ("A"), "1") == 0); > SELF_CHECK (strcmp (env.envp ()[0], "A=1") == 0); > SELF_CHECK (env.envp ()[1] == NULL); I didn't understand how it would work to conditionally define the pragma based on the compiler, but now that I see it I understand. LTGM. ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2] environ-selftests: Ignore -Wself-move warning 2017-06-22 10:57 ` Simon Marchi @ 2017-06-22 11:43 ` Pedro Alves 0 siblings, 0 replies; 29+ messages in thread From: Pedro Alves @ 2017-06-22 11:43 UTC (permalink / raw) To: Simon Marchi; +Cc: Simon Marchi, gdb-patches, sergiodj On 06/22/2017 11:57 AM, Simon Marchi wrote: > I didn't understand how it would work to conditionally define the pragma > based on the compiler, but now that I see it I understand. LTGM. Great, now pushed. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2017-06-25 10:58 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-21 20:15 [PATCH 0/4] Get rid of some more warnings given by clang Simon Marchi 2017-06-21 20:15 ` [PATCH 2/4] x86-dregs: Print debug registers one per line Simon Marchi 2017-06-21 20:31 ` Sergio Durigan Junior 2017-06-21 21:06 ` Simon Marchi 2017-06-25 10:58 ` Simon Marchi 2017-06-21 20:15 ` [PATCH 3/4] dtrace-probe: Put semicolon after while on its own line Simon Marchi 2017-06-21 20:34 ` Sergio Durigan Junior 2017-06-21 21:08 ` Simon Marchi 2017-06-21 21:36 ` Pedro Alves 2017-06-25 10:48 ` Simon Marchi 2017-06-25 10:57 ` Simon Marchi 2017-06-21 20:15 ` [PATCH 4/4] main: Don't add int to string Simon Marchi 2017-06-21 20:35 ` Sergio Durigan Junior 2017-06-25 10:58 ` Simon Marchi 2017-06-21 20:15 ` [PATCH 1/4] environ-selftests: Ignore -Wself-move warning Simon Marchi 2017-06-21 20:29 ` Sergio Durigan Junior 2017-06-21 21:05 ` Simon Marchi 2017-06-21 21:12 ` Sergio Durigan Junior 2017-06-21 21:28 ` Pedro Alves 2017-06-21 21:32 ` Sergio Durigan Junior 2017-06-22 7:44 ` Simon Marchi 2017-06-22 9:34 ` Pedro Alves 2017-06-21 21:16 ` Simon Marchi 2017-06-21 21:30 ` Sergio Durigan Junior 2017-06-22 8:31 ` [PATCH v2] " Simon Marchi 2017-06-22 9:51 ` Pedro Alves 2017-06-22 10:52 ` Pedro Alves 2017-06-22 10:57 ` Simon Marchi 2017-06-22 11:43 ` Pedro Alves
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 read-only IMAP folder(s) and NNTP newsgroup(s).