[00/55] Remove val_print

Message ID 20191208182958.10181-1-tom@tromey.com
Headers show
Series
  • Remove val_print
Related show

Message

Tom Tromey Dec. 8, 2019, 6:29 p.m.
I've long wanted to remove the val_print API.  It is used for the
lowest level of value printing, and works by passing around an
"exploded" value -- the type, offset, content, and address are passed
as separate parameters.  However, at some point we also had to start
passing around the original outer-most value as well (to check content
validity), which made the entire exercise a bit of a travesty.

I had put off this refactoring because I couldn't think of a
reviewable way to do it.  However, recently I realized I could
temporarily duplicate a lot of the code, rewriting as it made sense,
and then at the end flip a switch to enable the new paths, while
deleting the old API.  This is the approach taken by this series.

This is mildly ugly in two ways.

First, it duplicates code.  Of course, this is temporary, because the
duplication is entirely removed in the final patch.  So, I don't
consider it a big deal.

Second, there are some spots in the middle of this series that
introduce some regressions.  I tried to prevent this, but it turned
out to be difficult to do.  I am hoping that others agree with me that
this is ok in this case; but if not I can try again.

Patch #3 might not strictly be required.  It exists to preserve the
output in one particular case, but it's not clear if this is intended
behavior or just a historical accident.

It's possible that this series could result in too much memory
allocation when printing very large objects.  If this is a problem, I
think it's possible to implement content sharing in struct value --
one of the nice things about C++ is that it makes this sort of thing
more practical.

While this series eliminates most of the val_print code, a few
vestiges remain.  Some low-level printing routines for particular
types still accept old-style parameters.  This could be cleaned up,
but I haven't done so because this series is already too long.

There's a lot of duplication in the value-printing code.  For example,
the Pascal code seems to be a cut-and-paste of the C++ code; I doubt
(but don't really know) whether Pascal even has virtual base classes
to worry about.  So, another possible future direction would be to
share even more of this code across languages.

Regression tested by the buildbot.  I don't propose checking this in
until after the 9.1 branch has been made.

Let me know what you think.

Tom

Comments

Tom Tromey Jan. 15, 2020, 12:36 a.m. | #1
>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:


Tom> I've long wanted to remove the val_print API.  It is used for the
Tom> lowest level of value printing, and works by passing around an
Tom> "exploded" value -- the type, offset, content, and address are passed
Tom> as separate parameters.  However, at some point we also had to start
Tom> passing around the original outer-most value as well (to check content
Tom> validity), which made the entire exercise a bit of a travesty.

[...]

Tom> Regression tested by the buildbot.  I don't propose checking this in
Tom> until after the 9.1 branch has been made.

I'd like to move forward with this sometime soon, so if you have
comments or concerns, it would be good to send them.

thanks,
Tom
Simon Marchi Jan. 15, 2020, 6:15 a.m. | #2
On 2020-01-14 7:36 p.m., Tom Tromey wrote:
>>>>>> "Tom" == Tom Tromey <tom@tromey.com> writes:

> 

> Tom> I've long wanted to remove the val_print API.  It is used for the

> Tom> lowest level of value printing, and works by passing around an

> Tom> "exploded" value -- the type, offset, content, and address are passed

> Tom> as separate parameters.  However, at some point we also had to start

> Tom> passing around the original outer-most value as well (to check content

> Tom> validity), which made the entire exercise a bit of a travesty.

> 

> [...]

> 

> Tom> Regression tested by the buildbot.  I don't propose checking this in

> Tom> until after the 9.1 branch has been made.

> 

> I'd like to move forward with this sometime soon, so if you have

> comments or concerns, it would be good to send them.

> 

> thanks,

> Tom

> 


Hi Tom,

That looks very nice.  I remember struggling with the embedded offset concept,
I think passing down values everywhere makes it much easier to understand.

I left some small comments, but overall this LGTM.

Simon
Tom Tromey March 14, 2020, 12:02 a.m. | #3
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


[ removing val_print ]

Simon> I left some small comments, but overall this LGTM.

I've been rebasing this all along, and I re-regression tested it and
fixed one regression I found.  So, I'm going to check this in now.

Tom