[RFA] Emit a warning for ineffective set VAR = EXP command

[RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

Hi,

the 'set VAR=EXP' command is a real trap for Ada (and maybe other languages such as Pascal users), because the '=' is interpreted as BINOP_EQUAL instead of BINOP_ASSIGN.  You often do not realize that the current language is not C where you are using to command for registers or convenience variables.

I simply propose to emit a warning if the expression is not an assignment (or a comma expression).

No regressions on x86_64 GNU/Linux.

Ok for trunk ?

2012-04-27  Tristan Gingold  <gingold@adacore.com>

	* printcmd.c (set_command): Emit a warning if the expression is not
	an assignment.

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index d441dfe..79e38f2 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1080,6 +1080,12 @@ set_command (char *exp, int from_tty)
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
 
+  if (expr->nelts >= 1
+      && expr->elts[0].opcode != BINOP_ASSIGN
+      && expr->elts[0].opcode != BINOP_ASSIGN_MODIFY
+      && expr->elts[0].opcode != BINOP_COMMA)
+    warning (_("Expression is not an assignment (and might have no effect)"));
+
   evaluate_expression (expr);
   do_cleanups (old_chain);
 }

RE: [RFA] Emit a warning for ineffective set VAR = EXP command

[Pierre Muller]

  As pascal language maintainer
I fully support this RFA.

  When debugging mixed language executable I quite often
forget that I passed from a C compiled function to
a pascal compiled one and you the C version
resulting in the same kind of troubles
as Tristan does.



Pierre Muller
GDB pascal language maintainer



> -----Message d'origine-----
> De : gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] De la part de Tristan Gingold
> Envoyé : vendredi 27 avril 2012 15:29
> À : gdb-patches@sourceware.org ml
> Objet : [RFA] Emit a warning for ineffective set VAR = EXP command
> 
> Hi,
> 
> the 'set VAR=EXP' command is a real trap for Ada (and maybe other
languages
> such as Pascal users), because the '=' is interpreted as BINOP_EQUAL
instead
> of BINOP_ASSIGN.  You often do not realize that the current language is
not
> C where you are using to command for registers or convenience variables.
> 
> I simply propose to emit a warning if the expression is not an assignment
> (or a comma expression).
> 
> No regressions on x86_64 GNU/Linux.
> 
> Ok for trunk ?
> 
> 2012-04-27  Tristan Gingold  <gingold@adacore.com>
> 
> 	* printcmd.c (set_command): Emit a warning if the expression is not
> 	an assignment.
> 
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index d441dfe..79e38f2 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1080,6 +1080,12 @@ set_command (char *exp, int from_tty)
>    struct cleanup *old_chain =
>      make_cleanup (free_current_contents, &expr);
> 
> +  if (expr->nelts >= 1
> +      && expr->elts[0].opcode != BINOP_ASSIGN
> +      && expr->elts[0].opcode != BINOP_ASSIGN_MODIFY
> +      && expr->elts[0].opcode != BINOP_COMMA)
> +    warning (_("Expression is not an assignment (and might have no
> effect)"));
> +
>    evaluate_expression (expr);
>    do_cleanups (old_chain);
>  }

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tom Tromey]

>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:

Tristan> I simply propose to emit a warning if the expression is not an
Tristan> assignment (or a comma expression).

It seems reasonable to me.

Tristan> Ok for trunk ?

I think there should be a test case.

Tom

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On Apr 27, 2012, at 6:04 PM, Tom Tromey wrote:

>>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
> 
> Tristan> I simply propose to emit a warning if the expression is not an
> Tristan> assignment (or a comma expression).
> 
> It seems reasonable to me.
> 
> Tristan> Ok for trunk ?
> 
> I think there should be a test case.

Sure.  Is it ok with this testcase ?

Tristan.

gdb/testsuite/
2012-05-02  Tristan Gingold  <gingold@adacore.com>

	* gdb.base/set-noassign.exp: New test.

diff --git a/gdb/testsuite/gdb.base/set-noassign.exp b/gdb/testsuite/gdb.base/set-noassign.exp
new file mode 100644
index 0000000..d43bfbe
--- /dev/null
+++ b/gdb/testsuite/gdb.base/set-noassign.exp
@@ -0,0 +1,41 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# 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/>.
+
+
+set testfile start
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+    untested "Couldn't compile test program"
+    return -1
+}
+
+# Get things started.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+# Verify that set command without assignment emits a warning.
+#
+gdb_test "set x==3" \
+	"warning: Expression is not an assignment \\(and might have no effect\\)" \
+	"warning for set without assignment"

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Doug Evans]

I think the convention now is to use prepare_for_testing to simplify a
lot of the setup in the .exp file.  Grep for other uses to see how
it's used.

On Wed, May 2, 2012 at 6:17 AM, Tristan Gingold <gingold@adacore.com> wrote:
>
> On Apr 27, 2012, at 6:04 PM, Tom Tromey wrote:
>
>>>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
>>
>> Tristan> I simply propose to emit a warning if the expression is not an
>> Tristan> assignment (or a comma expression).
>>
>> It seems reasonable to me.
>>
>> Tristan> Ok for trunk ?
>>
>> I think there should be a test case.
>
> Sure.  Is it ok with this testcase ?
>
> Tristan.
>
> gdb/testsuite/
> 2012-05-02  Tristan Gingold  <gingold@adacore.com>
>
>        * gdb.base/set-noassign.exp: New test.
>
> diff --git a/gdb/testsuite/gdb.base/set-noassign.exp b/gdb/testsuite/gdb.base/set-noassign.exp
> new file mode 100644
> index 0000000..d43bfbe
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/set-noassign.exp
> @@ -0,0 +1,41 @@
> +# Copyright 2012 Free Software Foundation, Inc.
> +
> +# 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/>.
> +
> +
> +set testfile start
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
> +    untested "Couldn't compile test program"
> +    return -1
> +}
> +
> +# Get things started.
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}
> +
> +# Verify that set command without assignment emits a warning.
> +#
> +gdb_test "set x==3" \
> +       "warning: Expression is not an assignment \\(and might have no effect\\)" \
> +       "warning for set without assignment"
>

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 2, 2012, at 6:40 PM, Doug Evans wrote:

> I think the convention now is to use prepare_for_testing to simplify a
> lot of the setup in the .exp file.  Grep for other uses to see how
> it's used.

Ah thanks.  I didn't start from the best .exp file.

Ok with this version ?

testsuite/
2012-05-02  Tristan Gingold  <gingold@adacore.com>

	* gdb.base/set-noassign.exp: New test.


diff --git a/gdb/testsuite/gdb.base/set-noassign.exp b/gdb/testsuite/gdb.base/set-noassign.exp
new file mode 100644
index 0000000..c9971f5
--- /dev/null
+++ b/gdb/testsuite/gdb.base/set-noassign.exp
@@ -0,0 +1,32 @@
+# Copyright 2012 Free Software Foundation, Inc.
+
+# 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/>.
+
+
+if { [prepare_for_testing set-noassign.exp "set-noassign" start.c {debug nowarnings}] } {
+    return -1
+}
+
+if ![runto_main] then {
+    fail "Can't run to main"
+    return 0
+}
+
+# Verify that set command without assignment emits a warning.
+#
+gdb_test "set language ada" ".*" "set language ada"
+
+gdb_test "set x=3" \
+	"warning: Expression is not an assignment \\(and might have no effect\\)" \
+	"warning for set without assignment"

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Doug Evans]

On Thu, May 3, 2012 at 2:59 AM, Tristan Gingold <gingold@adacore.com> wrote:
>
> On May 2, 2012, at 6:40 PM, Doug Evans wrote:
>
>> I think the convention now is to use prepare_for_testing to simplify a
>> lot of the setup in the .exp file.  Grep for other uses to see how
>> it's used.
>
> Ah thanks.  I didn't start from the best .exp file.
>
> Ok with this version ?
>
> testsuite/
> 2012-05-02  Tristan Gingold  <gingold@adacore.com>
>
>        * gdb.base/set-noassign.exp: New test.
>
>
> diff --git a/gdb/testsuite/gdb.base/set-noassign.exp b/gdb/testsuite/gdb.base/set-noassign.exp
> new file mode 100644
> index 0000000..c9971f5
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/set-noassign.exp
> @@ -0,0 +1,32 @@
> +# Copyright 2012 Free Software Foundation, Inc.
> +
> +# 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/>.
> +
> +
> +if { [prepare_for_testing set-noassign.exp "set-noassign" start.c {debug nowarnings}] } {
> +    return -1
> +}
> +
> +if ![runto_main] then {
> +    fail "Can't run to main"
> +    return 0
> +}
> +
> +# Verify that set command without assignment emits a warning.
> +#
> +gdb_test "set language ada" ".*" "set language ada"
> +
> +gdb_test "set x=3" \
> +       "warning: Expression is not an assignment \\(and might have no effect\\)" \
> +       "warning for set without assignment"
>

Ok with one more change.

-gdb_test "set language ada" ".*" "set language ada"
+gdb_test_no_output "set language ada"

[I'm assuming the warning test has the desired affect.  I don't know ada. :-)]

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Joel Brobecker]

> Ok with one more change.
> 
> -gdb_test "set language ada" ".*" "set language ada"
> +gdb_test_no_output "set language ada"

There is going to be some output, because this command is forcing
a language which does not match the frame.

> [I'm assuming the warning test has the desired affect.  I don't know ada. :-)]

Correct, it's a `='/`==' vs `:='/'=' confusion. In Ada, the comparison
operator is "=". The user should have used the assignment operator which
is ":=".  The number of times I got bitten by this....

-- 
Joel

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Doug Evans]

On Thu, May 3, 2012 at 8:05 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>> Ok with one more change.
>>
>> -gdb_test "set language ada" ".*" "set language ada"
>> +gdb_test_no_output "set language ada"
>
> There is going to be some output, because this command is forcing
> a language which does not match the frame.

Ah.

>> [I'm assuming the warning test has the desired affect.  I don't know ada. :-)]
>
> Correct, it's a `='/`==' vs `:='/'=' confusion. In Ada, the comparison
> operator is "=". The user should have used the assignment operator which
> is ":=".  The number of times I got bitten by this....

It's trivial to add tests for a few more languages.
Whether it's worth it, I don't know.
It may be worth it if only as an educational vehicle so people who
don't know these other languages get exposed to them for this
particular issue.

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Joel Brobecker]

> It's trivial to add tests for a few more languages.
> Whether it's worth it, I don't know.
> It may be worth it if only as an educational vehicle so people who
> don't know these other languages get exposed to them for this
> particular issue.

I can extend the testcase to add a test with C, and perhaps we could
convince Pierre to add something for Pascal... Let's allow Tristan
to commit his patch, and then we can add to it?

-- 
Joel

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Doug Evans]

On Thu, May 3, 2012 at 3:04 PM, Joel Brobecker <brobecker@adacore.com> wrote:
>> It's trivial to add tests for a few more languages.
>> Whether it's worth it, I don't know.
>> It may be worth it if only as an educational vehicle so people who
>> don't know these other languages get exposed to them for this
>> particular issue.
>
> I can extend the testcase to add a test with C, and perhaps we could
> convince Pierre to add something for Pascal... Let's allow Tristan
> to commit his patch, and then we can add to it?

Either order works for me (and on whatever schedule works for you).

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 3, 2012, at 6:33 PM, Doug Evans wrote:

> On Thu, May 3, 2012 at 8:05 AM, Joel Brobecker <brobecker@adacore.com> wrote:
>>> Ok with one more change.
>>> 
>>> -gdb_test "set language ada" ".*" "set language ada"
>>> +gdb_test_no_output "set language ada"
>> 
>> There is going to be some output, because this command is forcing
>> a language which does not match the frame.
> 
> Ah.

Now committed.

>>> [I'm assuming the warning test has the desired affect.  I don't know ada. :-)]
>> 
>> Correct, it's a `='/`==' vs `:='/'=' confusion. In Ada, the comparison
>> operator is "=". The user should have used the assignment operator which
>> is ":=".  The number of times I got bitten by this....
> 
> It's trivial to add tests for a few more languages.
> Whether it's worth it, I don't know.

Well I don't think it is worth.  Gdb testsuite is not really a pedagogic material :-)

> It may be worth it if only as an educational vehicle so people who
> don't know these other languages get exposed to them for this
> particular issue.

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Joel Brobecker]

> Ah thanks.  I didn't start from the best .exp file.

The ration of bad-old-cruft vs up-to-date is very bad, so I try to
avoid starting from random .exp files (I usually start from a few
of mine that are recent and were reviewed by others).

Here is a link to our Testcase Cookbook:
http://sourceware.org/gdb/wiki/GDBTestcaseCookbook

-- 
Joel

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 3, 2012, at 5:02 PM, Joel Brobecker wrote:

>> Ah thanks.  I didn't start from the best .exp file.
> 
> The ration of bad-old-cruft vs up-to-date is very bad, so I try to
> avoid starting from random .exp files (I usually start from a few
> of mine that are recent and were reviewed by others).
> 
> Here is a link to our Testcase Cookbook:
> http://sourceware.org/gdb/wiki/GDBTestcaseCookbook

I wasn't aware of this page.  Maybe it should be the default home gdb page :-)

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Maciej W. Rozycki]

On Fri, 27 Apr 2012, Tristan Gingold wrote:

> the 'set VAR=EXP' command is a real trap for Ada (and maybe other 
> languages such as Pascal users), because the '=' is interpreted as 
> BINOP_EQUAL instead of BINOP_ASSIGN.  You often do not realize that the 
> current language is not C where you are using to command for registers 
> or convenience variables.
> 
> I simply propose to emit a warning if the expression is not an 
> assignment (or a comma expression).
> 
> No regressions on x86_64 GNU/Linux.
> 
> Ok for trunk ?
> 
> 2012-04-27  Tristan Gingold  <gingold@adacore.com>
> 
> 	* printcmd.c (set_command): Emit a warning if the expression is not
> 	an assignment.

 It does regress gdb.base/freebpcmd.exp apparently:

Breakpoint 1, main (argc=1, argv=0xbffff904) at .../gdb/testsuite/gdb.base/freebpcmd.c:27
27          printf (">>> %d\n", i); /* euphonium */
"odd "$79 = 39
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
warning: Expression is not an assignment (and might have no effect)
ERROR: internal buffer is full.
UNRESOLVED: gdb.base/freebpcmd.exp: run program with breakpoint commands

This warns about "set variable $j++" presumably -- should the warning be 
disabled for pre/post increments/decrements?

  Maciej

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 4, 2012, at 9:01 PM, Maciej W. Rozycki wrote:

> On Fri, 27 Apr 2012, Tristan Gingold wrote:
> 
>> the 'set VAR=EXP' command is a real trap for Ada (and maybe other 
>> languages such as Pascal users), because the '=' is interpreted as 
>> BINOP_EQUAL instead of BINOP_ASSIGN.  You often do not realize that the 
>> current language is not C where you are using to command for registers 
>> or convenience variables.
>> 
>> I simply propose to emit a warning if the expression is not an 
>> assignment (or a comma expression).
>> 
>> No regressions on x86_64 GNU/Linux.
>> 
>> Ok for trunk ?
>> 
>> 2012-04-27  Tristan Gingold  <gingold@adacore.com>
>> 
>> 	* printcmd.c (set_command): Emit a warning if the expression is not
>> 	an assignment.
> 
> It does regress gdb.base/freebpcmd.exp apparently:
> 
> Breakpoint 1, main (argc=1, argv=0xbffff904) at .../gdb/testsuite/gdb.base/freebpcmd.c:27
> 27          printf (">>> %d\n", i); /* euphonium */
> "odd "$79 = 39
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> warning: Expression is not an assignment (and might have no effect)
> ERROR: internal buffer is full.
> UNRESOLVED: gdb.base/freebpcmd.exp: run program with breakpoint commands
> 
> This warns about "set variable $j++" presumably -- should the warning be 
> disabled for pre/post increments/decrements?

I am not opposed to disable warnings for pre/post inc/dec.
But this usage is dubious (the help explicitly mentions VAR=EXP !)

Opinion ?

Tristan.

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tom Tromey]

>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:

Tristan> I am not opposed to disable warnings for pre/post inc/dec.
Tristan> But this usage is dubious (the help explicitly mentions VAR=EXP !)

Tristan> Opinion ?

Either way is fine by me.
I suppose it seems a little pedantic to warn about the ++ operator.

Tom

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Paul_Koning]

On May 7, 2012, at 3:35 PM, Tom Tromey wrote:

>>>>>> "Tristan" == Tristan Gingold <gingold@adacore.com> writes:
> 
> Tristan> I am not opposed to disable warnings for pre/post inc/dec.
> Tristan> But this usage is dubious (the help explicitly mentions VAR=EXP !)
> 
> Tristan> Opinion ?
> 
> Either way is fine by me.
> I suppose it seems a little pedantic to warn about the ++ operator.
> 
> Tom

What would ++ do if your current language is Ada, or Fortran?  Complain about syntax error?

It would be good to clarify the documentation to say that this also works.

	paul

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tom Tromey]

>>>>> "Paul" ==   <Paul_Koning@Dell.com> writes:

Paul> What would ++ do if your current language is Ada, or Fortran?
Paul> Complain about syntax error?

The expression is parsed according to the current language.
So, yes, syntax error, assuming this is invalid in Ada or Fortran.

Tom

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Joel Brobecker]

> > This warns about "set variable $j++" presumably -- should the warning be 
> > disabled for pre/post increments/decrements?
> 
> I am not opposed to disable warnings for pre/post inc/dec.
> But this usage is dubious (the help explicitly mentions VAR=EXP !)
> 
> Opinion ?

I think we should avoid the warning for pre/post inc/dec. This
type of expression might be a little outside the method proposed
in our documentation, but I think it's still a perfectly valid
expression that results in an assignment being performed.

-- 
Joel

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 7, 2012, at 9:38 PM, Joel Brobecker wrote:

>>> This warns about "set variable $j++" presumably -- should the warning be 
>>> disabled for pre/post increments/decrements?
>> 
>> I am not opposed to disable warnings for pre/post inc/dec.
>> But this usage is dubious (the help explicitly mentions VAR=EXP !)
>> 
>> Opinion ?
> 
> I think we should avoid the warning for pre/post inc/dec. This
> type of expression might be a little outside the method proposed
> in our documentation, but I think it's still a perfectly valid
> expression that results in an assignment being performed.

I don't know who should approve this adjustment, but here is the version that deals with pre/post inc/dec.
Note that it still warns for expressions such as i++ * 2.

Tested on set-nowarns.exp.

Tristan.

2012-05-09  Tristan Gingold  <gingold@adacore.com>

	* printcmd.c (set_command): Add pre/post inc/dec.

diff --git a/gdb/printcmd.c b/gdb/printcmd.c
index 79e38f2..fa76296 100644
--- a/gdb/printcmd.c
+++ b/gdb/printcmd.c
@@ -1080,11 +1080,21 @@ set_command (char *exp, int from_tty)
   struct cleanup *old_chain =
     make_cleanup (free_current_contents, &expr);
 
-  if (expr->nelts >= 1
-      && expr->elts[0].opcode != BINOP_ASSIGN
-      && expr->elts[0].opcode != BINOP_ASSIGN_MODIFY
-      && expr->elts[0].opcode != BINOP_COMMA)
-    warning (_("Expression is not an assignment (and might have no effect)"));
+  if (expr->nelts >= 1)
+    switch (expr->elts[0].opcode)
+      {
+      case UNOP_PREINCREMENT:
+      case UNOP_POSTINCREMENT:
+      case UNOP_PREDECREMENT:
+      case UNOP_POSTDECREMENT:
+      case BINOP_ASSIGN:
+      case BINOP_ASSIGN_MODIFY:
+      case BINOP_COMMA:
+	break;
+      default:
+	warning
+	  (_("Expression is not an assignment (and might have no effect)"));
+      }
 
   evaluate_expression (expr);
   do_cleanups (old_chain);

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Paul_Koning]

On May 9, 2012, at 8:28 AM, Tristan Gingold wrote:

> 
> On May 7, 2012, at 9:38 PM, Joel Brobecker wrote:
> 
>>>> This warns about "set variable $j++" presumably -- should the warning be 
>>>> disabled for pre/post increments/decrements?
>>> 
>>> I am not opposed to disable warnings for pre/post inc/dec.
>>> But this usage is dubious (the help explicitly mentions VAR=EXP !)
>>> 
>>> Opinion ?
>> 
>> I think we should avoid the warning for pre/post inc/dec. This
>> type of expression might be a little outside the method proposed
>> in our documentation, but I think it's still a perfectly valid
>> expression that results in an assignment being performed.
> 
> I don't know who should approve this adjustment, but here is the version that deals with pre/post inc/dec.
> Note that it still warns for expressions such as i++ * 2.

If you had it walk through the elts[] list, would it then work for that case?

	paul

> 
> Tested on set-nowarns.exp.
> 
> Tristan.
> 
> 2012-05-09  Tristan Gingold  <gingold@adacore.com>
> 
> 	* printcmd.c (set_command): Add pre/post inc/dec.
> 
> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
> index 79e38f2..fa76296 100644
> --- a/gdb/printcmd.c
> +++ b/gdb/printcmd.c
> @@ -1080,11 +1080,21 @@ set_command (char *exp, int from_tty)
>   struct cleanup *old_chain =
>     make_cleanup (free_current_contents, &expr);
> 
> -  if (expr->nelts >= 1
> -      && expr->elts[0].opcode != BINOP_ASSIGN
> -      && expr->elts[0].opcode != BINOP_ASSIGN_MODIFY
> -      && expr->elts[0].opcode != BINOP_COMMA)
> -    warning (_("Expression is not an assignment (and might have no effect)"));
> +  if (expr->nelts >= 1)
> +    switch (expr->elts[0].opcode)
> +      {
> +      case UNOP_PREINCREMENT:
> +      case UNOP_POSTINCREMENT:
> +      case UNOP_PREDECREMENT:
> +      case UNOP_POSTDECREMENT:
> +      case BINOP_ASSIGN:
> +      case BINOP_ASSIGN_MODIFY:
> +      case BINOP_COMMA:
> +	break;
> +      default:
> +	warning
> +	  (_("Expression is not an assignment (and might have no effect)"));
> +      }
> 
>   evaluate_expression (expr);
>   do_cleanups (old_chain);
> 

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 9, 2012, at 3:53 PM, <Paul_Koning@Dell.com> <Paul_Koning@Dell.com> wrote:

> 
> On May 9, 2012, at 8:28 AM, Tristan Gingold wrote:
> 
>> 
>> On May 7, 2012, at 9:38 PM, Joel Brobecker wrote:
>> 
>>>>> This warns about "set variable $j++" presumably -- should the warning be 
>>>>> disabled for pre/post increments/decrements?
>>>> 
>>>> I am not opposed to disable warnings for pre/post inc/dec.
>>>> But this usage is dubious (the help explicitly mentions VAR=EXP !)
>>>> 
>>>> Opinion ?
>>> 
>>> I think we should avoid the warning for pre/post inc/dec. This
>>> type of expression might be a little outside the method proposed
>>> in our documentation, but I think it's still a perfectly valid
>>> expression that results in an assignment being performed.
>> 
>> I don't know who should approve this adjustment, but here is the version that deals with pre/post inc/dec.
>> Note that it still warns for expressions such as i++ * 2.
> 
> If you had it walk through the elts[] list, would it then work for that case?

Is it worth the burden ?

> 
> 	paul
> 
>> 
>> Tested on set-nowarns.exp.
>> 
>> Tristan.
>> 
>> 2012-05-09  Tristan Gingold  <gingold@adacore.com>
>> 
>> 	* printcmd.c (set_command): Add pre/post inc/dec.
>> 
>> diff --git a/gdb/printcmd.c b/gdb/printcmd.c
>> index 79e38f2..fa76296 100644
>> --- a/gdb/printcmd.c
>> +++ b/gdb/printcmd.c
>> @@ -1080,11 +1080,21 @@ set_command (char *exp, int from_tty)
>>  struct cleanup *old_chain =
>>    make_cleanup (free_current_contents, &expr);
>> 
>> -  if (expr->nelts >= 1
>> -      && expr->elts[0].opcode != BINOP_ASSIGN
>> -      && expr->elts[0].opcode != BINOP_ASSIGN_MODIFY
>> -      && expr->elts[0].opcode != BINOP_COMMA)
>> -    warning (_("Expression is not an assignment (and might have no effect)"));
>> +  if (expr->nelts >= 1)
>> +    switch (expr->elts[0].opcode)
>> +      {
>> +      case UNOP_PREINCREMENT:
>> +      case UNOP_POSTINCREMENT:
>> +      case UNOP_PREDECREMENT:
>> +      case UNOP_POSTDECREMENT:
>> +      case BINOP_ASSIGN:
>> +      case BINOP_ASSIGN_MODIFY:
>> +      case BINOP_COMMA:
>> +	break;
>> +      default:
>> +	warning
>> +	  (_("Expression is not an assignment (and might have no effect)"));
>> +      }
>> 
>>  evaluate_expression (expr);
>>  do_cleanups (old_chain);
>> 
> 

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Maciej W. Rozycki]

On Wed, 9 May 2012, Paul_Koning@Dell.com wrote:

> >> I think we should avoid the warning for pre/post inc/dec. This
> >> type of expression might be a little outside the method proposed
> >> in our documentation, but I think it's still a perfectly valid
> >> expression that results in an assignment being performed.
> > 
> > I don't know who should approve this adjustment, but here is the version that deals with pre/post inc/dec.
> > Note that it still warns for expressions such as i++ * 2.
> 
> If you had it walk through the elts[] list, would it then work for that case?

 What do you mean by "work" here?  I think a warning for "i++ * 2" is 
expected as that's questionable use -- the result of the multiplication is 
discarded.  Did you mean anything else?

 I can't approve this change, but it seems OK to me.

  Maciej

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Paul_Koning]

On May 9, 2012, at 10:12 AM, Maciej W. Rozycki wrote:

> On Wed, 9 May 2012, Paul_Koning@Dell.com wrote:
> 
>>>> I think we should avoid the warning for pre/post inc/dec. This
>>>> type of expression might be a little outside the method proposed
>>>> in our documentation, but I think it's still a perfectly valid
>>>> expression that results in an assignment being performed.
>>> 
>>> I don't know who should approve this adjustment, but here is the version that deals with pre/post inc/dec.
>>> Note that it still warns for expressions such as i++ * 2.
>> 
>> If you had it walk through the elts[] list, would it then work for that case?
> 
> What do you mean by "work" here?  I think a warning for "i++ * 2" is 
> expected as that's questionable use -- the result of the multiplication is 
> discarded.  Did you mean anything else?

You're right, but it does perform an assignment (to "i") so a message saying that the statement has no effect is not accurate.  Then again, given that the statement doesn't make much sense, the fact that the message is not completely accurate isn't all that interesting -- unlike the simple "i++" it isn't a likely case in reality.

	paul

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Maciej W. Rozycki]

On Wed, 9 May 2012, Paul_Koning@Dell.com wrote:

> > What do you mean by "work" here?  I think a warning for "i++ * 2" is 
> > expected as that's questionable use -- the result of the multiplication is 
> > discarded.  Did you mean anything else?
> 
> You're right, but it does perform an assignment (to "i") so a message 
> saying that the statement has no effect is not accurate.  Then again, 
> given that the statement doesn't make much sense, the fact that the 
> message is not completely accurate isn't all that interesting -- unlike 
> the simple "i++" it isn't a likely case in reality.

 I think it's all right -- "might have no effect" is weaker than "has no 
effect" and will draw one's attention in the case of a typo or suchlike.

  Maciej

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Paul_Koning]

On May 9, 2012, at 11:38 AM, Maciej W. Rozycki wrote:

> On Wed, 9 May 2012, Paul_Koning@Dell.com wrote:
> 
>>> What do you mean by "work" here?  I think a warning for "i++ * 2" is 
>>> expected as that's questionable use -- the result of the multiplication is 
>>> discarded.  Did you mean anything else?
>> 
>> You're right, but it does perform an assignment (to "i") so a message 
>> saying that the statement has no effect is not accurate.  Then again, 
>> given that the statement doesn't make much sense, the fact that the 
>> message is not completely accurate isn't all that interesting -- unlike 
>> the simple "i++" it isn't a likely case in reality.
> 
> I think it's all right -- "might have no effect" is weaker than "has no 
> effect" and will draw one's attention in the case of a typo or suchlike.
> 
>  Maciej

Good point.  So I'll agree that the proposed change is fine (and no, I can't approve it either).

	paul

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Joel Brobecker]

> I don't know who should approve this adjustment, but here is the
> version that deals with pre/post inc/dec.  Note that it still warns
> for expressions such as i++ * 2.

I think a warning is fine in this case. "set i++ * 2" almost certainly
means something is fishy, because the multiplication is useless.

> 2012-05-09  Tristan Gingold  <gingold@adacore.com>
> 
> 	* printcmd.c (set_command): Add pre/post inc/dec.

This looks good to me.

Thanks Tristan,
-- 
Joel

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Tristan Gingold]

On May 9, 2012, at 4:17 PM, Joel Brobecker wrote:

>> I don't know who should approve this adjustment, but here is the
>> version that deals with pre/post inc/dec.  Note that it still warns
>> for expressions such as i++ * 2.
> 
> I think a warning is fine in this case. "set i++ * 2" almost certainly
> means something is fishy, because the multiplication is useless.
> 
>> 2012-05-09  Tristan Gingold  <gingold@adacore.com>
>> 
>> 	* printcmd.c (set_command): Add pre/post inc/dec.
> 
> This looks good to me.

Now committed.

Tristan.

Re: [RFA] Emit a warning for ineffective set VAR = EXP command

[Maciej W. Rozycki]

On Mon, 7 May 2012, Tristan Gingold wrote:

> > This warns about "set variable $j++" presumably -- should the warning be 
> > disabled for pre/post increments/decrements?
> 
> I am not opposed to disable warnings for pre/post inc/dec.
> But this usage is dubious (the help explicitly mentions VAR=EXP !)
> 
> Opinion ?

 I think this is a bit too pedantic, there's no doubt these operators 
imply an assignment in C.  I think the help text is clear enough with:

"Evaluate expression EXP and assign result to variable VAR, using 
assignment syntax appropriate for the current language (VAR = EXP or VAR 
:= EXP for example)."

-- it quotes examples but these are by no means exhaustive and one can 
imply any assignment valid according to the language selected is going to 
be accepted.  And there's quite a bunch of assignment operators defined 
for C.  Note that this help text does not mention "VAR <<= EXP" either and 
there's no doubt this is a valid assignment in C too.  If you're concerned 
about this then perhaps the offline manual could be more elaborate.

 I think one of our principles is to make debugging fast and efficient 
rather than picky about the input style and with that in mind we should 
accept any reasonable input from the user, sometimes even where it is not 
perfectly valid for the given language, e.g. you can ask GDB like this:

(gdb) p/x (char[4])somevar

to print SOMEVAR (e.g. an INT) as a 4-element character array even though 
this is really not something the C compiler would normally accept.  I 
recall seeing this principle written down somewhere, but cannot track it 
down right now.

 I use such constructs all the time, especially when referring to CPU 
registers and I think it would be an unnecessary burden if rather than:

(gdb) set $a0++

I had to write:

(gdb) set $a0 += 1

or maybe even:

(gdb) set $a0 = $a0 + 1

That's IMHO an unnecessary waste of time, keyboard, etc. (note that += is 
actually explicitly mentioned in the manual).

  Maciej