[patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa-2
Hi,

The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.

No regressions on linux32.


Thanks,
Anton

dynamic_show_str3.ChangeLog (1K) Download Attachment
dynamic_show_str3.diff (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa
Ping, could anybody review the patch?


Thanks,
Anton

-------- Original message --------
> Hi,
>
> The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.
>
> No regressions on linux32.
>
>
> Thanks,
> Anton


Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa
In reply to this post by xgsa-2
Ping! Please, review the patch.

Anton.

-------- Original message --------
> Hi,
>
> The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.
>
> No regressions on linux32.
>
>
> Thanks,
> Anton


Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

Joel Brobecker
> Ping! Please, review the patch.

I tried, a couple of times. But in the end, I couldn't find enough
time to understand that part of the code and approve. My guess
is that Tom has the best knowlege of the area. I'd wait until
he comes back...

--
Joel
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

Eli Zaretskii
In reply to this post by xgsa-2
> From: xgsa <[hidden email]>
> Envelope-From: [hidden email]
> Date: Mon, 23 Apr 2012 00:09:57 +0400
>
>
> The documentation for -var-create says that "for a varobj whose type is some sort of aggregate (e.g., a struct), or for a dynamic varobj, the 'value' attribute will not be interesting". That's not true, as dynamic varobj could also have no children (so its value will be not "{...}" and will be interesting). However I think the string value of varobj that has pretty printer installed should always be returned via MI. It could contain such a useful information as container length or reference count for smart pointers. Moreover, it seems that this information cannot be accessed via MI in another way. This patch contains the proposed fix, documentation and test suite update.

The documentation part is fine with me.

Thanks.
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa
In reply to this post by Joel Brobecker
Tom, it seems you are here. Please, look through this path when you have
time.

Thanks,
Anton.

-------- Original message --------
>> Ping! Please, review the patch.
> I tried, a couple of times. But in the end, I couldn't find enough
> time to understand that part of the code and approve. My guess
> is that Tom has the best knowlege of the area. I'd wait until
> he comes back...
>


Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa
Ping! I am still asking about the review. It should not be too long. The
patch contains only a few changes in code and test suite fixture for it.

Thanks,
Anton

-------- Original message --------

> Tom, it seems you are here. Please, look through this path when you
> have time.
>
> Thanks,
> Anton.
>
> -------- Original message --------
>>> Ping! Please, review the patch.
>> I tried, a couple of times. But in the end, I couldn't find enough
>> time to understand that part of the code and approve. My guess
>> is that Tom has the best knowlege of the area. I'd wait until
>> he comes back...
>>
>
>
>
>


Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

Tom Tromey
In reply to this post by xgsa-2
>>>>> "Anton" == xgsa  <[hidden email]> writes:

Anton> The documentation for -var-create says that "for a varobj whose type
Anton> is some sort of aggregate (e.g., a struct), or for a dynamic varobj,
Anton> the 'value' attribute will not be interesting". That's not true, as
Anton> dynamic varobj could also have no children (so its value will be not
Anton> "{...}" and will be interesting). However I think the string value of
Anton> varobj that has pretty printer installed should always be returned via
Anton> MI. It could contain such a useful information as container length or
Anton> reference count for smart pointers. Moreover, it seems that this
Anton> information cannot be accessed via MI in another way. This patch
Anton> contains the proposed fix, documentation and test suite update.

The patch itself looks fine, but I am not sure whether it should go in.

My recollection is that the code originally worked this way, but
Vladimir asked for the "{...}" behavior specifically.

I've CC'd him for comments.

I am in favor of this change, as I never understood the current
behavior; so in the absence of comments I will approve it.

I'm sorry about the delay.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

André Pönitz
On Tue, May 15, 2012 at 10:28:25AM -0600, Tom Tromey wrote:

> >>>>> "Anton" == xgsa  <[hidden email]> writes:
>
> Anton> The documentation for -var-create says that "for a varobj whose type
> Anton> is some sort of aggregate (e.g., a struct), or for a dynamic varobj,
> Anton> the 'value' attribute will not be interesting". That's not true, as
> Anton> dynamic varobj could also have no children (so its value will be not
> Anton> "{...}" and will be interesting). However I think the string value of
> Anton> varobj that has pretty printer installed should always be returned via
> Anton> MI. It could contain such a useful information as container length or
> Anton> reference count for smart pointers. Moreover, it seems that this
> Anton> information cannot be accessed via MI in another way. This patch
> Anton> contains the proposed fix, documentation and test suite update.
>
> The patch itself looks fine, but I am not sure whether it should go in.
>
> My recollection is that the code originally worked this way, but
> Vladimir asked for the "{...}" behavior specifically.

It might affect frontends taking a "{...}" as indication that there
are childrens. In fact, I just found some code using that as a shortcut.

The safe way would be to add a new field 'formatted-value' or similar
with the new value, and perhaps even announce the feature using
-list-features, but I doubt this is worth the trouble.

I am not sure how frontend developers interpret "27.4 gdb/mi Development
and Front Ends", or how much they rely on it in general.

The list of "to be expexted changes" mentions
  * New MI commands may be added.
  * New fields may be added to the output of any MI command.
  * The range of values for fields with specified values, e.g., in_scope
    (see -var-update) may be extended.

It's not directly obviously to me that this list is meant to be
exhaustive, and even if, whether the third bullet point covers
changing existing values, but there is precedence that changing
values was prefered over adding a new field in the past, so there
is no real reason to not do it again.

Andre'
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

Tom Tromey
>>>>> "André" == André Pönitz <[hidden email]> writes:

Tom> My recollection is that the code originally worked this way, but
Tom> Vladimir asked for the "{...}" behavior specifically.

André> It might affect frontends taking a "{...}" as indication that there
André> are childrens. In fact, I just found some code using that as a shortcut.

That code is just plain broken, though.  The has_more field conveys this
information.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

André Pönitz
On Wed, May 16, 2012 at 08:58:42AM -0600, Tom Tromey wrote:

> >>>>> <[hidden email]> writes:
>
> Tom> My recollection is that the code originally worked this way, but
> Tom> Vladimir asked for the "{...}" behavior specifically.
>
> André> It might affect frontends taking a "{...}" as indication that
> André> there are childrens. In fact, I just found some code using that
> André> as a shortcut.
>
> That code is just plain broken, though.  The has_more field conveys
> this information.

If you say so.

'git blame' thinks you added it in September 2009, so gdb 7.0 is
probably the first one that has it.  Calling code "broken" just
because it handles (also...) "legacy" setups is a bit of a stretch
in my opinion.

In any case, I was merely trying to convey the idea that an
unsuspecting frontend implementor might read the documentation
in a way that would be incompatible with the change.

I am personally not affected by any change in the -var-*
output, so maybe I should not have bothered to comment at all.

If you really want to read more into the comment than what was
actually written, then perhaps a friendly nudge to adapt the
documentation to existing practice.

Andre'
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

Vladimir Prus-3
On 17/05/12 16:19, André Pönitz wrote:

> On Wed, May 16, 2012 at 08:58:42AM -0600, Tom Tromey wrote:
>>>>>>> <[hidden email]>  writes:
>>
>> Tom>  My recollection is that the code originally worked this way, but
>> Tom>  Vladimir asked for the "{...}" behavior specifically.
>>
>> André>  It might affect frontends taking a "{...}" as indication that
>> André>  there are childrens. In fact, I just found some code using that
>> André>  as a shortcut.
>>
>> That code is just plain broken, though.  The has_more field conveys
>> this information.
>
> If you say so.
>
> 'git blame' thinks you added it in September 2009, so gdb 7.0 is
> probably the first one that has it.  Calling code "broken" just
> because it handles (also...) "legacy" setups is a bit of a stretch
> in my opinion.
>
> In any case, I was merely trying to convey the idea that an
> unsuspecting frontend implementor might read the documentation
> in a way that would be incompatible with the change.
>
> I am personally not affected by any change in the -var-*
> output, so maybe I should not have bothered to comment at all.
>
> If you really want to read more into the comment than what was
> actually written, then perhaps a friendly nudge to adapt the
> documentation to existing practice.

I has been quite a while, but I think I was primarily concerned that GDB would abuse value string on varobj
to show what GDB thinks is the right rendering of data. E.g. while "[5]" as top-level value might be a reasonable
rendition for an array with 5 children, it's not entirely clear why GDB's machine interface should take liberties
at telling frontend how to render that. Neither "[", nor 5, nor "]" is integral part of data, and as soon as
GDB outputs that, frontend has no way of knowing whether this is GDB trying to be helpful, or really interesting
data that cannot be obtained in any other way.

--
Vladimir Prus
CodeSourcery / Mentor Graphics
http://www.mentor.com/embedded-software/
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

Tom Tromey
In reply to this post by André Pönitz
>>>>> "André" == André Pönitz <[hidden email]> writes:

André> 'git blame' thinks you added it in September 2009, so gdb 7.0 is
André> probably the first one that has it.  Calling code "broken" just
André> because it handles (also...) "legacy" setups is a bit of a stretch
André> in my opinion.

We're only talking about dynamic varobjs here, which the MI user must
explicitly request.  Checking a dynamic varobj for "{...}" has always
been wrong, since they were first introduced.

Looking at this for non-dynamic varobjs... well, I wouldn't do that, but
it isn't relevant.

André> If you really want to read more into the comment than what was
André> actually written, then perhaps a friendly nudge to adapt the
André> documentation to existing practice.

As far as I know the documentation is correct.

Tom
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa
In reply to this post by Vladimir Prus-3
> I has been quite a while, but I think I was primarily concerned that
> GDB would abuse value string on varobj
> to show what GDB thinks is the right rendering of data. E.g. while
> "[5]" as top-level value might be a reasonable
> rendition for an array with 5 children, it's not entirely clear why
> GDB's machine interface should take liberties
> at telling frontend how to render that. Neither "[", nor 5, nor "]" is
> integral part of data, and as soon as
> GDB outputs that, frontend has no way of knowing whether this is GDB
> trying to be helpful, or really interesting
> data that cannot be obtained in any other way.

I cannot figure out why the frontend need to know it? I think, it should
just output what GDB returns, cause GDB knows better how to visualize
the concrete data type due to pretty printers mechanism. Otherwise, the
frontend should have something similar to pretty printers that will
visualize different types in a comfortable way. In any case if the
implemented behavior is not desirable the string() method of pretty
printer could be left unimplemented (or return None). It will give the
behavior you described above, but could also be extended if necessary.


Thanks,
Anton
Reply | Threaded
Open this post in threaded view
|

Re: [patch] Use the string returned by pretty printer for MI varobjs instead of "{...}"

xgsa
So could I check this patch in or not? I have written my arguments for
these changes, Vladimir has written his ones against them. It seems Tom
in favor of these changes too. So what is the final decision? Could
somebody say "these changes are approved" or "no, these changes should
not go in" or what should be done in such cases?

Thanks,
Anton

-------- Original message --------

>> I has been quite a while, but I think I was primarily concerned that
>> GDB would abuse value string on varobj
>> to show what GDB thinks is the right rendering of data. E.g. while
>> "[5]" as top-level value might be a reasonable
>> rendition for an array with 5 children, it's not entirely clear why
>> GDB's machine interface should take liberties
>> at telling frontend how to render that. Neither "[", nor 5, nor "]"
>> is integral part of data, and as soon as
>> GDB outputs that, frontend has no way of knowing whether this is GDB
>> trying to be helpful, or really interesting
>> data that cannot be obtained in any other way.
>
> I cannot figure out why the frontend need to know it? I think, it
> should just output what GDB returns, cause GDB knows better how to
> visualize the concrete data type due to pretty printers mechanism.
> Otherwise, the frontend should have something similar to pretty
> printers that will visualize different types in a comfortable way. In
> any case if the implemented behavior is not desirable the string()
> method of pretty printer could be left unimplemented (or return None).
> It will give the behavior you described above, but could also be
> extended if necessary.
>
>
> Thanks,
> Anton
>
>