Re: [patch][python] Implement Python lazy strings (PR 10705)

[Phil Muldoon <pmuldoon@redhat.com>]

On 01/11/2010 09:08 PM, Tom Tromey wrote:
>>>>>> "Phil" == Phil Muldoon <pmuldoon@redhat.com> writes:
> 
> Phil> Index: gdb/varobj.c
> [...]
> Phil> +		if (is_lazy_string (output))
> Phil> +		    thevalue = extract_lazy_string (output, &type,
> Phil> +						    &len, &encoding);
> Phil> +		else
> Phil>  		  {
> Phil> -		    char *s = PyString_AsString (py_str);
> Phil> -		    len = PyString_Size (py_str);
> Phil> -		    thevalue = xmemdup (s, len + 1, len + 1);
> Phil> -		    Py_DECREF (py_str);
> Phil> +		    PyObject *py_str
> Phil> +		      = python_string_to_target_python_string (output);
> Phil> +		    if (py_str)
> Phil> +		      {
> Phil> +			char *s = PyString_AsString (py_str);
> Phil> +			len = PyString_Size (py_str);
> Phil> +			thevalue = xmemdup (s, len + 1, len + 1);
> Phil> +			type = builtin_type (gdbarch)->builtin_char;
> Phil> +			Py_DECREF (py_str);
> Phil> +		      }
> Phil>  		  }
> Phil>  		Py_DECREF (output);
> Phil>  	      }
> Phil>  	    if (thevalue && !string_print)
> Phil>  	      {
> Phil>  		do_cleanups (back_to);
> Phil> +		xfree (encoding);
> Phil>  		return thevalue;
> 
> This is wrong in the lazy string case, because you are returning raw
> bytes, but the user expects them to be interpreted according to the
> encoding.
> 
> We discussed this on irc and I thought the result was that we agreed
> that in this case we would pretend that a "string" hint was given.

We did. This case is slightly different case from original case we
discussed in irc (the original was in the non-mi case).  Anyway, if
thevalue is not NULL (and in these cases it won't) it is printed via
LA_PRINT_STRING (just a few lines further past this patch hunk) and
the encoding happens at that point.


> 
> Phil> +PyObject *
> Phil> +gdbpy_create_lazy_string_object (CORE_ADDR address, long length,
> Phil> +			   const char *encoding, struct type *type)
> Phil> +{
> [...]
> Phil> +  if (!str_obj)
> Phil> +      return NULL;
> 
> Indentation looks wrong on the second line.

Ok.

> Phil> +/* Determine whether the printer object pointed to by OBJ is a
> Phil> +   Python lazy string.  */
> Phil> +int
> Phil> +is_lazy_string (PyObject *result)
> Phil> +{
> Phil> +  return PyObject_TypeCheck (result, &lazy_string_object_type);
> Phil> +}
> 
> Why here and not in py-lazy-string.c?
> Then lazy_string_object_type could be static.

Ok.

> Phil> +/* Extract and return the actual string from the lazy string object
> Phil> +   STRING.  Additionally, the string type is written to *STR_TYPE, the
> Phil> +   string length is written to *LENGTH, and the string encoding is
> Phil> +   written to *ENCODING.  On error, NULL is returned.  The caller is
> Phil> +   responsible for freeing the returned buffer.  */
> Phil> +gdb_byte *
> Phil> +extract_lazy_string (PyObject *string, struct type **str_type,
> Phil> +		     long *length, char **encoding)
> 
> Likewise.

Ok.

> Phil> +  output = convert_value_from_python (string);
> 
> Why do we need to do this?
> Can't we just get the address directly?

As a lazy string is a pointer, the code needs to actually find the
type associated with the characters in the string. So later in the
code:

 if (TYPE_CODE (value_type (output)) == TYPE_CODE_PTR)
    output = value_ind (output);

I originally though about doing this in the creation of a lazy
string. But if that value is processed through common_val_print (say
in non a pretty printing case) the output for "foo" would be:

"f"

But if it is left as a  pointer type it prints as:

"foo"

I'm not adverse to performing the value_ind action in the creation of the
lazy string. I just thought it would be nice to allow lazy strings to
print by common_val_print as well as our specialized case.

> 
> Phil> +  TRY_CATCH (except, RETURN_MASK_ALL)
> Phil> +    {
> Phil> +
> 
> Extra blank line.
> 
> Phil> +  if (except.reason < 0)
> Phil> +    {
> Phil> +
> 
> Likewise.
> 
> Phil> +}
> Phil> +
> Phil> +
> Phil> +
> 
> Likewise.
> 
> Phil> +      int is_lazy = 0;
> Phil> +
> Phil> +      is_lazy = is_lazy_string (py_str);
> 
> There's no need to initialize is_lazy to 0 if you then immediately
> assign to it.

Ok.

>
> Variables like 'encoding' that are only used in one branch of the if
> should just be declared in that branch.

Ok. Mea culpa on the obvious mistakes ;)

Cheers

Phil