Re: [python][patch] Preserve string length, and embedded-nulls in pretty-print string emission.

[Phil Muldoon <pmuldoon@redhat.com>]

On 06/11/2009 06:11 PM, Tom Tromey wrote:


Hi Tom

Thanks for the comments. Most are done, but I have some further 
questions with others.


> Phil>  +  int is_string = 0;
>
> Phil>  -	  if (gdbpy_is_string (result))
> Phil>  -	    output = python_string_to_host_string (result);
> Phil>  -	  else if (PyObject_TypeCheck (result,&value_object_type))
> Phil>  +	  is_string = gdbpy_is_string (result);
> Phil>  +	  if (! is_string)
>
> I think the previous code was clearer.  Don't introduce is_string,
> instead...
>    


Agreed, fixed.


> Phil>  +	      Py_DECREF (result);
>
> Set 'result = NULL' here.
>    


Likewise


> Phil>  +unicode_to_encoded_string (PyObject *unicode_str, const char *charset, int *size)
>
> [...]
>
> Phil>  +  str_size = PyString_Size (string) + 1;
> Phil>  +  result = xmemdup (PyString_AsString (string), str_size, str_size);
> Phil>  +  if (size != NULL)
> Phil>  +    *size = str_size;
>
> How about a variant of this function that just returns the
> intermediate 'string' PyObject?
>    

To avoid the use of an out-parameter? I can see the use of returning an 
encoded version of the the unicode PyObject string, but if you wanted 
that, why would you just not call:

PyUnicode_AsEncodedString directly? Wouldn't the following:

static PyObject*
unicode_to_encoded_string (PyObject *unicode_str, const char *charset)
{
return PyUnicode_AsEncodedString (unicode_str, charset, NULL);
}

Be the same equivalent? What would the purpose of a new variant achieve 
there?


> Phil>  +      output = python_string_to_target_string (py_str,&len);
> Phil>  +      if (output)
> Phil>  +	{
> Phil>  +	  struct cleanup *old = make_cleanup (xfree, output);
>
> Adding a new unicode_to_target_string variant (or
> python_string_to_target_string, or whatever is nicest) means you can
> avoid an allocation here.
>    


Similar to above, I'm not sure what you mean here. 
python_string_to_target_string does more than than the previous hunk 
comment, but eventually will have to be converted to char * to pass 
through the LA_PRINT_STRING that is directly below this code?



> Phil>  --- a/gdb/varobj.c
> Phil>  +	    Py_DECREF(py_str);
>
> Space before open paren.
>
>    

Fixed thanks!

Regards

Phil