diff mbox

[v2] environ-selftests: Ignore -Wself-move warning

Message ID 1498120280-17996-1-git-send-email-simon.marchi@ericsson.com
State New, archived
Headers

Commit Message

Simon Marchi June 22, 2017, 8:31 a.m. UTC 
  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

Comments

Pedro Alves June 22, 2017, 9:51 a.m. UTC | #1 
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
diff mbox

Patch

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