diff mbox

[1/4] environ-selftests: Ignore -Wself-move warning

Message ID 1498076108-29914-2-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi June 21, 2017, 8:15 p.m. UTC 
  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(+)

Comments

Sergio Durigan Junior June 21, 2017, 8:29 p.m. UTC | #1 
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,
Simon Marchi June 21, 2017, 9:05 p.m. UTC | #2 
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?
Sergio Durigan Junior June 21, 2017, 9:11 p.m. UTC | #3 
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".
Simon Marchi June 21, 2017, 9:16 p.m. UTC | #4 
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 :).
Pedro Alves June 21, 2017, 9:28 p.m. UTC | #5 
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
Sergio Durigan Junior June 21, 2017, 9:29 p.m. UTC | #6 
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 Durigan Junior June 21, 2017, 9:32 p.m. UTC | #7 
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.
Simon Marchi June 22, 2017, 7:44 a.m. UTC | #8 
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
Pedro Alves June 22, 2017, 9:34 a.m. UTC | #9 
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
diff mbox

Patch

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