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

Phil Muldoon
2010-01-11T21:30:28Z

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