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

[Phil Muldoon <pmuldoon@redhat.com>]

Tom Tromey wrote:
> Phil> +convert_value_from_python (PyObject *obj, int *size)
>
> I don't understand why this needs the additional argument.
> It seems to me that the size is implicit in the returned value.
>   


This function returns a struct value. If that value contains a string 
with embedded nulls, is there a way to determine the size of the 
"gdb_byte *contents"? I might have missed a method - or the obvious - 
but this is the reason for the out parameter. If the parameter size is 
not NULL, the size of the string is returned there.


> Phil> +	  if (PyErr_Occurred ())
> Phil> +	      *out_value = NULL;
>
> This looks like it is indented improperly.
>   


Ok, thanks.


> Phil> +  str_length = pretty_print_one_value (printer, &replacement);
> Phil> +  if (replacement)
> Phil> +    if (str_length > 0)
> Phil> +      {
> Phil> +	LA_GET_STRING (replacement, &output, &str_length, &la_encoding);
> Phil> +	if (hint && !strcmp (hint, "string"))
> Phil> +	  LA_PRINT_STRING (stream, output, str_length, 1, 0, options);
> Phil> +	else
> Phil> +	  fputs_filtered (output, stream);
> Phil> +	xfree (output);
>
> Why do we need LA_GET_STRING here?  Aren't the string contents already
> in the value?
>   

I'll look at this a little more closely. But I use this to extract a 
string with embedded nulls with a concrete length. Do I misread your 
thoughts here? Or are you suggesting we just pass in value->contents to 
LA_PRINT_STRING?

> Also, why check "str_length > 0".  A string might have length 0.
> I think instead you probably want some kind of type check.
>   

Ok, thanks.

> Phil>    *replacement = NULL;
> Phil> -  result = pretty_print_one_value (printer_obj, replacement);
> Phil> -  if (result == NULL);
> Phil> +  size = pretty_print_one_value (printer_obj, replacement);
> Phil> +
> Phil> +  if (replacement == NULL);
> Phil>      gdbpy_print_stack ();
>
> This should check *replacement; 'replacement' can never be NULL.
>   


Ok, thanks.

> Phil> +std::string cplus_str ("embedded\0null\0string",20);
>
> A test relying on std::string is not very robust -- std::string may
> change over time, breaking the test.  Instead, it is better to write
> custom test code and a printer to match.  That way, we control the
> implementation.
>   


Ok thanks for the pointer. Can you please elaborate a bit more?

Regards

Phil