[patch][python] Fix python/14513

[patch][python] Fix python/14513

[Phil Muldoon]

This patch fixes a bug for silent parameters that do not print anything
when a parameter is set.  Currently if a parameter is silent, a
newline will be erroneously outputted.

E.g.

(gdb) set extended-prompt (New Prompt)

(New Prompt)

Parameters normally report what the value has been set too:

(gdb) set foobar 5
foobar has been set to 5.

This patch just checks the string length of the output string before
printing it.

OK?

Cheers,

Phil

2013-09-18  Phil Muldoon  <pmuldoon@redhat.com>

        PR python/14513

	* python/py-param.c (get_set_value): Check doc_string length
	before outputting to console.

--

diff --git a/gdb/python/py-param.c b/gdb/python/py-param.c
index 9f56c3a..921acae 100644
--- a/gdb/python/py-param.c
+++ b/gdb/python/py-param.c
@@ -389,7 +389,9 @@ get_set_value (char *args, int from_tty,
     }
 
   make_cleanup (xfree, set_doc_string);
-  fprintf_filtered (gdb_stdout, "%s\n", set_doc_string);
+
+  if (strlen (set_doc_string) > 0)
+    fprintf_filtered (gdb_stdout, "%s\n", set_doc_string);
 
   Py_XDECREF (set_doc_func);
   do_cleanups (cleanup);

	

Re: [patch][python] Fix python/14513

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> Parameters normally report what the value has been set too:
Phil> (gdb) set foobar 5
Phil> foobar has been set to 5.

Phil> This patch just checks the string length of the output string before
Phil> printing it.

Phil> OK?

Built-in parameters don't seem to do this printing:

    (gdb) set print elements 500
    (gdb) 

I think the Python layer should follow this precedent.
Maybe get_set_string can be entirely removed.

Tom

Re: [patch][python] Fix python/14513

[Phil Muldoon]

On 18/09/13 21:26, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> Parameters normally report what the value has been set too:
> Phil> (gdb) set foobar 5
> Phil> foobar has been set to 5.
> 
> Phil> This patch just checks the string length of the output string before
> Phil> printing it.
> 
> Phil> OK?
> 
> Built-in parameters don't seem to do this printing:
> 
>     (gdb) set print elements 500
>     (gdb) 
> 
> I think the Python layer should follow this precedent.
> Maybe get_set_string can be entirely removed.

If you look at the testsuite for py-param, they do have this option.
I have no opinion about this, but if we remove it I think we will we
be breaking our API promise?  This functionality has been in there
for awhile.

Here is an example from the testsuite:

class TestParam (gdb.Parameter):
"""When enabled, test param does something useful. When disabled, does nothing."""

   show_doc = "Show the state of the boolean test-param"
   set_doc = "Set the state of the boolean test-param" 

   def get_show_string (self, pvalue):" ""\
       return "The state of the Test Parameter is " + pvalue
   
   def get_set_string (self):
       val = "on"
       if (self.value == False):
          val = "off"
       return "Test Parameter has been set to " + val
	  
   def __init__ (self, name):
       super (TestParam, self).__init__ (name, gdb.COMMAND_DATA, gdb.PARAM_BOOLEAN)
       self.value = True

test_param = TestParam ('print test-param')

Notice that get_set_string, a callback, has a return value.  I felt it
best in these circumstances to maintain API compatibility and do the
empty string check.

What do you think?

Cheers,

Phil

Re: [patch][python] Fix python/14513

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> If you look at the testsuite for py-param, they do have this option.

Oh right!  I forgot about that, sorry.

Phil> What do you think?

Ugh.  Well, while we don't actually promise to print anything, it seems
weird to require a string result just to throw it away.  How about we
remove the "else" clause and not print anything at all if the
get_set_string method is not implemented?  This ought to clean up most
uses.

Tom

Re: [patch][python] Fix python/14513

[Phil Muldoon]

On 18/09/13 21:40, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> If you look at the testsuite for py-param, they do have this option.
> 
> Oh right!  I forgot about that, sorry.
> 
> Phil> What do you think?
> 
> Ugh.  Well, while we don't actually promise to print anything, it seems
> weird to require a string result just to throw it away.  How about we
> remove the "else" clause and not print anything at all if the
> get_set_string method is not implemented?  This ought to clean up most
> uses.

I think you mean the else clause in the py-param.c code?  If so, we
cannot remove it as it preserves < 7.3 API:

  else
    {
      /* We have to preserve the existing < GDB 7.3 API.  If a
	 callback function does not exist, then attempt to read the
	 set_doc attribute.  */
      set_doc_string  = get_doc_string (obj, set_doc_cst);
    }

I am not sure what the resolution is here.  At some point we have to
decide whether we have anything valid to print.  We can hoist the
fprint into both of these branches, but even in the "else" clause we
have to check if we actually have anything as the fprint adds a
newline to the set documentation:

fprintf_filtered (gdb_stdout, "%s\n", set_doc_string);

What are your thoughts on this?

Cheers,

Phil

Re: [patch][python] Fix python/14513

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

Phil> I am not sure what the resolution is here.  At some point we have to
Phil> decide whether we have anything valid to print.  We can hoist the
Phil> fprint into both of these branches, but even in the "else" clause we
Phil> have to check if we actually have anything as the fprint adds a
Phil> newline to the set documentation:

Phil> fprintf_filtered (gdb_stdout, "%s\n", set_doc_string);

Phil> What are your thoughts on this?

I've been thinking about it more and I don't understand why we want to
print anything in the "set" command.
I think it's fine if we just don't call a method.
What do you think of that?

Tom

Re: [patch][python] Fix python/14513

[Phil Muldoon]

On 21/10/13 22:58, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> I am not sure what the resolution is here.  At some point we have to
> Phil> decide whether we have anything valid to print.  We can hoist the
> Phil> fprint into both of these branches, but even in the "else" clause we
> Phil> have to check if we actually have anything as the fprint adds a
> Phil> newline to the set documentation:
> 
> Phil> fprintf_filtered (gdb_stdout, "%s\n", set_doc_string);
> 
> Phil> What are your thoughts on this?
> 
> I've been thinking about it more and I don't understand why we want to
> print anything in the "set" command.
> I think it's fine if we just don't call a method.
> What do you think of that?

I am fine with it, as long as you are.

Cheers,

Phil

Re: [patch][python] Fix python/14513

[Tom Tromey]

>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:

>> I've been thinking about it more and I don't understand why we want to
>> print anything in the "set" command.
>> I think it's fine if we just don't call a method.
>> What do you think of that?

Phil> I am fine with it, as long as you are.

Yeah, let's drop it if possible.

Tom