* [PATCH] Fix GCC6 -Wmisleading-indentation issues.
@ 2016-01-25 19:41 Mark Wielaard
2016-01-25 21:46 ` Simon Marchi
0 siblings, 1 reply; 6+ messages in thread
From: Mark Wielaard @ 2016-01-25 19:41 UTC (permalink / raw)
To: gdb-patches; +Cc: Mark Wielaard
GCC6 will warn about misleading indentation issues like:
gdb/ada-lang.c: In function ‘ada_evaluate_subexp’:
ada-lang.c:11423:9: error: statement is indented as if it were guarded by...
arg1 = unwrap_value (arg1);
^~~~
gdb/ada-lang.c:11421:7: note: ...this ‘else’ clause, but it is not
else
^~~~
In this case it would be a bug except for the fact the if clause already
returned early. So this misindented statement really only got executed
for the else case. But it could easily mislead a reader, so adding a
proper else block is the correct solution.
In case of c-typeprint.c (c_type_print_base) the if statement is indeed
misleadingly indented, but not a bug. Just indent correctly. The inflow.c
(terminal_ours_1) misindented block comes from the removal of an if clause
in commit d9d2d8b which looks correct. So just reindent the brackets of
the block. The linux-record.c misleadingly indented return statements
are just that. Misleading to the reader, but not actual bugs. So just
properly indent them so they don't look the fall under the wrong if clause.
---
gdb/ChangeLog | 11 +++++++++++
gdb/ada-lang.c | 8 +++++---
gdb/c-typeprint.c | 42 +++++++++++++++++++++---------------------
gdb/inflow.c | 4 ++--
gdb/linux-record.c | 8 ++++----
5 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 476bb5e..cf86e42 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2016-01-25 Mark Wielaard <mjw@redhat.com>
+
+ * ada-lang.c (ada_evaluate_subexp): Add proper else block.
+ * c-typeprint.c (c_type_print_base): Fix misleading indentation of
+ if statement.
+ * inflow.c (child_terminal_ours_1): Fix misleading indentation of
+ statement block.
+ * linux-record.c (record_linux_sockaddr): Fix misleading indentation
+ of return statements.
+ (record_linux_msghdr): Likewise.
+
2016-01-25 Pedro Alves <palves@redhat.com>
PR threads/19461
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 242e632..d874129 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -11419,9 +11419,11 @@ ada_evaluate_subexp (struct type *expect_type, struct expression *exp,
return value_zero (ada_aligned_type (type), lval_memory);
}
else
- arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
- arg1 = unwrap_value (arg1);
- return ada_to_fixed_value (arg1);
+ {
+ arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
+ arg1 = unwrap_value (arg1);
+ return ada_to_fixed_value (arg1);
+ }
case OP_TYPE:
/* The value is not supposed to be used. This is here to make it
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 762c027..6b9e6b3 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -1305,27 +1305,27 @@ c_type_print_base (struct type *type, struct ui_file *stream,
if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
fprintf_filtered (stream, "\n");
- for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
- {
- struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
-
- /* Dereference the typedef declaration itself. */
- gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
- target = TYPE_TARGET_TYPE (target);
-
- print_spaces_filtered (level + 4, stream);
- fprintf_filtered (stream, "typedef ");
-
- /* We want to print typedefs with substitutions
- from the template parameters or globally-known
- typedefs but not local typedefs. */
- c_print_type (target,
- TYPE_TYPEDEF_FIELD_NAME (type, i),
- stream, show - 1, level + 4,
- &semi_local_flags);
- fprintf_filtered (stream, ";\n");
- }
- }
+ for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
+ {
+ struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
+
+ /* Dereference the typedef declaration itself. */
+ gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
+ target = TYPE_TARGET_TYPE (target);
+
+ print_spaces_filtered (level + 4, stream);
+ fprintf_filtered (stream, "typedef ");
+
+ /* We want to print typedefs with substitutions
+ from the template parameters or globally-known
+ typedefs but not local typedefs. */
+ c_print_type (target,
+ TYPE_TYPEDEF_FIELD_NAME (type, i),
+ stream, show - 1, level + 4,
+ &semi_local_flags);
+ fprintf_filtered (stream, ";\n");
+ }
+ }
fprintfi_filtered (level, stream, "}");
}
diff --git a/gdb/inflow.c b/gdb/inflow.c
index bbd2b12..d1dd41c 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
return;
- {
+ {
#ifdef SIGTTOU
/* Ignore this signal since it will happen when we try to set the
pgrp. */
@@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
result = fcntl (0, F_SETFL, our_terminal_info.tflags);
result = fcntl (0, F_SETFL, our_terminal_info.tflags);
#endif
- }
+ }
}
/* Per-inferior data key. */
diff --git a/gdb/linux-record.c b/gdb/linux-record.c
index b9e49f3..29e299d 100644
--- a/gdb/linux-record.c
+++ b/gdb/linux-record.c
@@ -112,7 +112,7 @@ record_linux_sockaddr (struct regcache *regcache,
"memory at addr = 0x%s len = %d.\n",
phex_nz (len, tdep->size_pointer),
tdep->size_int);
- return -1;
+ return -1;
}
addrlen = (int) extract_unsigned_integer (a, tdep->size_int, byte_order);
if (addrlen <= 0 || addrlen > tdep->size_sockaddr)
@@ -150,7 +150,7 @@ record_linux_msghdr (struct regcache *regcache,
"len = %d.\n",
phex_nz (addr, tdep->size_pointer),
tdep->size_msghdr);
- return -1;
+ return -1;
}
/* msg_name msg_namelen */
@@ -188,7 +188,7 @@ record_linux_msghdr (struct regcache *regcache,
"len = %d.\n",
phex_nz (addr,tdep->size_pointer),
tdep->size_iovec);
- return -1;
+ return -1;
}
tmpaddr = (CORE_ADDR) extract_unsigned_integer (iov,
tdep->size_pointer,
@@ -983,7 +983,7 @@ Do you want to stop the program?"),
"memory at addr = 0x%s len = %d.\n",
OUTPUT_REG (tmpulongest, tdep->arg2),
tdep->size_ulong);
- return -1;
+ return -1;
}
tmpulongest = extract_unsigned_integer (a, tdep->size_ulong,
byte_order);
--
2.5.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix GCC6 -Wmisleading-indentation issues.
2016-01-25 19:41 [PATCH] Fix GCC6 -Wmisleading-indentation issues Mark Wielaard
@ 2016-01-25 21:46 ` Simon Marchi
2016-01-25 22:14 ` Mark Wielaard
0 siblings, 1 reply; 6+ messages in thread
From: Simon Marchi @ 2016-01-25 21:46 UTC (permalink / raw)
To: Mark Wielaard, gdb-patches
On 16-01-25 02:40 PM, Mark Wielaard wrote:
> GCC6 will warn about misleading indentation issues like:
>
> gdb/ada-lang.c: In function ‘ada_evaluate_subexp’:
> ada-lang.c:11423:9: error: statement is indented as if it were guarded by...
> arg1 = unwrap_value (arg1);
> ^~~~
>
> gdb/ada-lang.c:11421:7: note: ...this ‘else’ clause, but it is not
> else
> ^~~~
>
> In this case it would be a bug except for the fact the if clause already
> returned early. So this misindented statement really only got executed
> for the else case. But it could easily mislead a reader, so adding a
> proper else block is the correct solution.
>
> In case of c-typeprint.c (c_type_print_base) the if statement is indeed
> misleadingly indented, but not a bug. Just indent correctly. The inflow.c
> (terminal_ours_1) misindented block comes from the removal of an if clause
> in commit d9d2d8b which looks correct. So just reindent the brackets of
> the block. The linux-record.c misleadingly indented return statements
> are just that. Misleading to the reader, but not actual bugs. So just
> properly indent them so they don't look the fall under the wrong if clause.
Thanks for doing this!
If you feel like it, you could build with --enable-targets=all to enable more code.
You'll find more of these.
One little comment below, otherwise it LGTM (without implying that my voice is
worth anything).
> diff --git a/gdb/inflow.c b/gdb/inflow.c
> index bbd2b12..d1dd41c 100644
> --- a/gdb/inflow.c
> +++ b/gdb/inflow.c
> @@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
> if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
> return;
>
> - {
> + {
> #ifdef SIGTTOU
> /* Ignore this signal since it will happen when we try to set the
> pgrp. */
> @@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
> result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> #endif
> - }
> + }
Shouldn't the code inside the brackets be de-indented too?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix GCC6 -Wmisleading-indentation issues.
2016-01-25 21:46 ` Simon Marchi
@ 2016-01-25 22:14 ` Mark Wielaard
2016-01-25 22:24 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Mark Wielaard @ 2016-01-25 22:14 UTC (permalink / raw)
To: Simon Marchi; +Cc: gdb-patches
On Mon, Jan 25, 2016 at 04:46:10PM -0500, Simon Marchi wrote:
> If you feel like it, you could build with --enable-targets=all to
> enable more code.
> You'll find more of these.
Thanks, but honestly this was just inspired by my desire to get
a local build with gcc6 going. It would be good to build more code
with GCC6 of course. I am sure it will find interesting issues.
> One little comment below, otherwise it LGTM (without implying that my voice is
> worth anything).
>
> > diff --git a/gdb/inflow.c b/gdb/inflow.c
> > index bbd2b12..d1dd41c 100644
> > --- a/gdb/inflow.c
> > +++ b/gdb/inflow.c
> > @@ -413,7 +413,7 @@ child_terminal_ours_1 (int output_only)
> > if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
> > return;
> >
> > - {
> > + {
> > #ifdef SIGTTOU
> > /* Ignore this signal since it will happen when we try to set the
> > pgrp. */
> > @@ -497,7 +497,7 @@ child_terminal_ours_1 (int output_only)
> > result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> > result = fcntl (0, F_SETFL, our_terminal_info.tflags);
> > #endif
> > - }
> > + }
>
> Shouldn't the code inside the brackets be de-indented too?
Possibly. Or remove the whole block and move stuff into the outer scope
now that it isn't strictly needed. But both create lots of code movement
because of the peculiar ifdefs in this code. And it clearly wasn't the
intention of the original author that removed the if statement in
commit d9d2d8b.
Thanks,
Mark
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix GCC6 -Wmisleading-indentation issues.
2016-01-25 22:14 ` Mark Wielaard
@ 2016-01-25 22:24 ` Pedro Alves
2016-01-25 22:38 ` Mark Wielaard
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2016-01-25 22:24 UTC (permalink / raw)
To: Mark Wielaard, Simon Marchi; +Cc: gdb-patches
On 01/25/2016 10:14 PM, Mark Wielaard wrote:
> On Mon, Jan 25, 2016 at 04:46:10PM -0500, Simon Marchi wrote:
>> Shouldn't the code inside the brackets be de-indented too?
>
> Possibly. Or remove the whole block and move stuff into the outer scope
> now that it isn't strictly needed. But both create lots of code movement
> because of the peculiar ifdefs in this code. And it clearly wasn't the
> intention of the original author that removed the if statement in
> commit d9d2d8b.
Yeah... It was part being lazy and port not wanting to mess git blame for
such old code much. There's an easy way around it though. Add an
else, like:
if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
return;
+ else
{
Or, reverse the condition, like:
- if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
- return;
+ if (tinfo->run_terminal == NULL && gdb_has_a_terminal ())
{
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix GCC6 -Wmisleading-indentation issues.
2016-01-25 22:24 ` Pedro Alves
@ 2016-01-25 22:38 ` Mark Wielaard
2016-01-25 23:00 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Mark Wielaard @ 2016-01-25 22:38 UTC (permalink / raw)
To: Pedro Alves; +Cc: Simon Marchi, gdb-patches
[-- Attachment #1: Type: text/plain, Size: 428 bytes --]
On Mon, Jan 25, 2016 at 10:24:45PM +0000, Pedro Alves wrote:
> Yeah... It was part being lazy and port not wanting to mess git blame for
> such old code much. There's an easy way around it though. Add an
> else, like:
>
> if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
> return;
> + else
> {
O, very nice. I did it that way and adjusted commit message and ChangeLog.
OK to commit?
Thanks,
Mark
[-- Attachment #2: 0001-Fix-GCC6-Wmisleading-indentation-issues.patch --]
[-- Type: text/plain, Size: 7034 bytes --]
From 072fc44cafdff4517fd284da425ac3a8d1ff6343 Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mjw@redhat.com>
Date: Mon, 25 Jan 2016 20:29:54 +0100
Subject: [PATCH] Fix GCC6 -Wmisleading-indentation issues.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
GCC6 will warn about misleading indentation issues like:
gdb/ada-lang.c: In function ‘ada_evaluate_subexp’:
ada-lang.c:11423:9: error: statement is indented as if it were guarded by...
arg1 = unwrap_value (arg1);
^~~~
gdb/ada-lang.c:11421:7: note: ...this ‘else’ clause, but it is not
else
^~~~
In this case it would be a bug except for the fact the if clause already
returned early. So this misindented statement really only got executed
for the else case. But it could easily mislead a reader, so adding a
proper else block is the correct solution.
In case of c-typeprint.c (c_type_print_base) the if statement is indeed
misleadingly indented, but not a bug. Just indent correctly. The inflow.c
(terminal_ours_1) misindented block comes from the removal of an if clause
in commit d9d2d8b which looks correct. Just introduce an else to fixup the
indentation of the block. The linux-record.c misleadingly indented return
statements are just that. Misleading to the reader, but not actual bugs.
Just unindent them so they don't look like they fall under the wrong if
clause.
---
gdb/ChangeLog | 11 +++++++++++
gdb/ada-lang.c | 8 +++++---
gdb/c-typeprint.c | 42 +++++++++++++++++++++---------------------
gdb/inflow.c | 2 +-
gdb/linux-record.c | 8 ++++----
5 files changed, 42 insertions(+), 29 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 476bb5e..8a4453c 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,14 @@
+2016-01-25 Mark Wielaard <mjw@redhat.com>
+
+ * ada-lang.c (ada_evaluate_subexp): Add proper else block.
+ * c-typeprint.c (c_type_print_base): Fix misleading indentation of
+ if statement.
+ * inflow.c (child_terminal_ours_1): Fix misleading indentation of
+ statement block by introducing an else.
+ * linux-record.c (record_linux_sockaddr): Fix misleading indentation
+ of return statements.
+ (record_linux_msghdr): Likewise.
+
2016-01-25 Pedro Alves <palves@redhat.com>
PR threads/19461
diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 242e632..d874129 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -11419,9 +11419,11 @@ ada_evaluate_subexp (struct type *expect_type, struct expression *exp,
return value_zero (ada_aligned_type (type), lval_memory);
}
else
- arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
- arg1 = unwrap_value (arg1);
- return ada_to_fixed_value (arg1);
+ {
+ arg1 = ada_value_struct_elt (arg1, &exp->elts[pc + 2].string, 0);
+ arg1 = unwrap_value (arg1);
+ return ada_to_fixed_value (arg1);
+ }
case OP_TYPE:
/* The value is not supposed to be used. This is here to make it
diff --git a/gdb/c-typeprint.c b/gdb/c-typeprint.c
index 762c027..6b9e6b3 100644
--- a/gdb/c-typeprint.c
+++ b/gdb/c-typeprint.c
@@ -1305,27 +1305,27 @@ c_type_print_base (struct type *type, struct ui_file *stream,
if (TYPE_NFIELDS (type) != 0 || TYPE_NFN_FIELDS (type) != 0)
fprintf_filtered (stream, "\n");
- for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
- {
- struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
-
- /* Dereference the typedef declaration itself. */
- gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
- target = TYPE_TARGET_TYPE (target);
-
- print_spaces_filtered (level + 4, stream);
- fprintf_filtered (stream, "typedef ");
-
- /* We want to print typedefs with substitutions
- from the template parameters or globally-known
- typedefs but not local typedefs. */
- c_print_type (target,
- TYPE_TYPEDEF_FIELD_NAME (type, i),
- stream, show - 1, level + 4,
- &semi_local_flags);
- fprintf_filtered (stream, ";\n");
- }
- }
+ for (i = 0; i < TYPE_TYPEDEF_FIELD_COUNT (type); i++)
+ {
+ struct type *target = TYPE_TYPEDEF_FIELD_TYPE (type, i);
+
+ /* Dereference the typedef declaration itself. */
+ gdb_assert (TYPE_CODE (target) == TYPE_CODE_TYPEDEF);
+ target = TYPE_TARGET_TYPE (target);
+
+ print_spaces_filtered (level + 4, stream);
+ fprintf_filtered (stream, "typedef ");
+
+ /* We want to print typedefs with substitutions
+ from the template parameters or globally-known
+ typedefs but not local typedefs. */
+ c_print_type (target,
+ TYPE_TYPEDEF_FIELD_NAME (type, i),
+ stream, show - 1, level + 4,
+ &semi_local_flags);
+ fprintf_filtered (stream, ";\n");
+ }
+ }
fprintfi_filtered (level, stream, "}");
}
diff --git a/gdb/inflow.c b/gdb/inflow.c
index bbd2b12..4c80dbd 100644
--- a/gdb/inflow.c
+++ b/gdb/inflow.c
@@ -412,7 +412,7 @@ child_terminal_ours_1 (int output_only)
if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
return;
-
+ else
{
#ifdef SIGTTOU
/* Ignore this signal since it will happen when we try to set the
diff --git a/gdb/linux-record.c b/gdb/linux-record.c
index b9e49f3..29e299d 100644
--- a/gdb/linux-record.c
+++ b/gdb/linux-record.c
@@ -112,7 +112,7 @@ record_linux_sockaddr (struct regcache *regcache,
"memory at addr = 0x%s len = %d.\n",
phex_nz (len, tdep->size_pointer),
tdep->size_int);
- return -1;
+ return -1;
}
addrlen = (int) extract_unsigned_integer (a, tdep->size_int, byte_order);
if (addrlen <= 0 || addrlen > tdep->size_sockaddr)
@@ -150,7 +150,7 @@ record_linux_msghdr (struct regcache *regcache,
"len = %d.\n",
phex_nz (addr, tdep->size_pointer),
tdep->size_msghdr);
- return -1;
+ return -1;
}
/* msg_name msg_namelen */
@@ -188,7 +188,7 @@ record_linux_msghdr (struct regcache *regcache,
"len = %d.\n",
phex_nz (addr,tdep->size_pointer),
tdep->size_iovec);
- return -1;
+ return -1;
}
tmpaddr = (CORE_ADDR) extract_unsigned_integer (iov,
tdep->size_pointer,
@@ -983,7 +983,7 @@ Do you want to stop the program?"),
"memory at addr = 0x%s len = %d.\n",
OUTPUT_REG (tmpulongest, tdep->arg2),
tdep->size_ulong);
- return -1;
+ return -1;
}
tmpulongest = extract_unsigned_integer (a, tdep->size_ulong,
byte_order);
--
2.5.0
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] Fix GCC6 -Wmisleading-indentation issues.
2016-01-25 22:38 ` Mark Wielaard
@ 2016-01-25 23:00 ` Pedro Alves
0 siblings, 0 replies; 6+ messages in thread
From: Pedro Alves @ 2016-01-25 23:00 UTC (permalink / raw)
To: Mark Wielaard; +Cc: Simon Marchi, gdb-patches
On 01/25/2016 10:38 PM, Mark Wielaard wrote:
> On Mon, Jan 25, 2016 at 10:24:45PM +0000, Pedro Alves wrote:
>> Yeah... It was part being lazy and port not wanting to mess git blame for
>> such old code much. There's an easy way around it though. Add an
>> else, like:
>>
>> if (tinfo->run_terminal != NULL || gdb_has_a_terminal () == 0)
>> return;
>> + else
>> {
>
> O, very nice. I did it that way and adjusted commit message and ChangeLog.
>
> OK to commit?
OK.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-01-25 23:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-25 19:41 [PATCH] Fix GCC6 -Wmisleading-indentation issues Mark Wielaard
2016-01-25 21:46 ` Simon Marchi
2016-01-25 22:14 ` Mark Wielaard
2016-01-25 22:24 ` Pedro Alves
2016-01-25 22:38 ` Mark Wielaard
2016-01-25 23:00 ` Pedro Alves
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox