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

[Tom Tromey <tromey at redhat dot com>]

>>>>> "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.

One way to do this would be to set "string_print = 1" in the lazy case.

The documentation for the "string" hint should mention this.

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.

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.

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.

Phil> +  output = convert_value_from_python (string);

Why do we need to do this?
Can't we just get the address directly?

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.

Phil> +	  gdb_byte *output = NULL;
Phil> +	  struct type *type;
Phil> +	  long length;
Phil> +	  char *encoding = NULL;
Phil> +
Phil> +	  if (is_lazy_string (py_v))
Phil> +	    {
Phil> +	      output = extract_lazy_string (py_v, &type, &length, &encoding);
Phil> +	      if (!output)
Phil> +		gdbpy_print_stack ();
Phil> +	      LA_PRINT_STRING (stream, type, output, length, encoding,
Phil> +			       0, options);
Phil> +	      xfree (encoding);
Phil> +	      xfree (output);

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

Tom