Support serialization as another type without casting #12712

sipa · 2018-03-17T23:34:24Z

With the new AsType function it's possible to serialize an object as another (compatible) type, and is intended for invoking the serializer of a parent class.

Writing AsType<Parent>(child) will work in any context:

READWRITE(AsType<Parent>(child))
s << AsType<Parent>(child)
s >> AsType<Parent>(child)

In case child is const, the result will be a reference to a const Parent type, resulting in const-correct behavior.

For now this primitive isn't very useful, as the constness is statically known in each of the instances (so a simple cast would work). However, in #10785 I plan to modify the serialization code to have a single implementation which works on a const object when serializing and non-const when deserializing. AsType behaves correctly in this case, maintaining the constness of the argument. It's also safer, as this only involves automatic type conversions, and no explicit casts.

With the new AsType function it's possible to serialize an object as another (compatible) type, and is intended for invoking the serializer of a parent class. Writing AsType<Parent>(child) will work in any context: * READWRITE(AsType<Parent>(child)) * s << AsType<Parent>(child) * s >> AsType<Parent>(child) In case child is const, the result will be a reference to a const Parent type, resulting in const-correct behavior.

jonasschnelli · 2018-03-18T05:03:54Z

utACK a6e5ddc

meshcollider · 2018-03-18T10:10:16Z

utACK a6e5ddc

donaloconnor · 2018-03-18T19:36:52Z

utACK a6e5ddc

jimpo · 2018-03-18T23:26:47Z

-0

This adds a new codebase specific function for which a perfectly adequate language-standard one exists. It's just one more thing for people new to the codebase to see and ask themselves "but why not static_cast"? Converting the invocations from *static_cast<X*>(this) to static_cast<X&>(*this) seems fine and marginally better though.

sipa · 2018-03-18T23:32:59Z

@jimpo That doesn't work when the constness of the argument isn't known.

I guess the context isn't entirely clear in this PR, but the goal is to have serialization functions that use single implementation that taken a const argument when serializing and non-const argument when deserializing. Simple static casts aren't enough, as they don't maintain constness, but I'd be happy to learn about another approach.

EDIT: I've added a rationale to the PR description; you're very right to point out it wasn't sufficient.

ryanofsky

utACK a6e5ddc, with caveat that I think @jimpo makes a good point about downsides of adding a new cast. IMO, it would be clearer to drop AsType and replace:

READWRITE(AsType<Parent>(child))
s << AsType<Parent>(child)
s >> AsType<Parent>(child)

with:

READWRITEAS(child, Parent)
s << static_cast<const Parent&>(child)
s >> static_cast<Parent&>(child)

I don't think AsType actually serves a useful function except when it's being used in combination with the READWRITE macro, and in that case, it could just be built in to the macro. In other contexts, AsType could even be dangerous since it potentially returns references to temporaries.

sipa · 2018-03-19T17:14:59Z

@ryanofsky I was trying to avoid as much macro magic as possible.

In other contexts, AsType could even be dangerous since it potentially returns references to temporaries.

Can you give an example?

ryanofsky · 2018-03-19T17:55:59Z

@ryanofsky I was trying to avoid as much macro magic as possible.

To be clear, I'm not suggesting replacing AsType with a macro, I'm just suggesting using it internally in READWRITEAS and not exposing it to callers.

I don't think macro magic calls like READWRITEAS(child, Parent) are great, but they seem preferable to a macro magic calls combined with template magic like READWRITE(AsType<Parent>(child)).

Can you give an example?

const Base& i = CastAs<Base>(Derived());

It just seems like the CastAs construct is unusual and unsafe and something that doesn't really make sense outside of the context of our READWRITE macro.

sipa · 2018-03-19T18:10:53Z

const Base& i = CastAs<Base>(Derived());

Sure, but that has nothing to do with the AsType construction, I think? This works just as well:

const Derived& i = Derived();

Or even:

const Base& i = Derived();

It just seems like the CastAs construct is unusual and unsafe and something that doesn't really make sense outside of the context of our READWRITE macro.


~~~My point is that `AsType` never makes unsafe constructions that were illegal legal, and is thus much less risky to use than `static_cast`.~~~

EDIT: I see your point. `const Base& i = Derived()` extends the lifetime of the temporary, while `const Base& i = AsType<Base>(Derived());` does not, making it unsafe to use.

ryanofsky · 2018-03-19T18:58:54Z

Yes, but it can't participate in a READWRITE with multiple arguments at once.

That's a good point. I can see how it is a benefit of the current approach. I'm not sure it actually justifies exposing the cast construct, but makes sense.

const Derived& i = Derived();

This is safe due to reference lifetime extension (https://abseil.io/tips/107). My example wasn't safe because lifetime extension can't work in that case. To be sure, I don't actually think this is a big deal. It's just a general reason to be wary of functions like AsType which return references that might not have clear lifetimes.

sipa · 2018-03-19T19:37:04Z

I'm not sure it actually justifies exposing the cast construct, but makes sense.

Right, I guess that's the question.

After the discussion above with @jimpo and @ryanofsky I'm fine with either the READWRITEAS(type, obj) or the READWRITE(AsType<type>(obj)) approach (this PR). If we choose the former, I'll close this PR and structure things differently.

ryanofsky · 2018-03-19T20:05:39Z

Can't speak for @jimpo. I prefer READWRITEAS to AsType, but I think you should choose whichever approach you like better.

jimpo · 2018-03-19T20:12:27Z

I also prefer READWRITEAS because it contains the scope of the logic. But if you go with AsType I would elaborate on the in-code comments to explain that it is designed for use with READWRITE and not as a general-purpose cast.

sipa · 2018-03-19T20:15:07Z

Thanks, good points.

I'll redo this.

818dc74 Support serialization as another type without casting (Pieter Wuille) Pull request description: This adds a `READWRITEAS(type, obj)` macro which serializes `obj` as if it were converted to `const type&` when `const`, and to `type&` when non-`const`. No actual cast is involved, so this only works when this conversion can be done automatically. This makes it usable in serialization code that uses a single implementation for both serialization and deserializing, which doesn't know the constness of the object involved. This is a redo of #12712, using a slightly different interface. Tree-SHA512: 262f0257284ff99b5ffaec9b997c194e221522ba35c3ac8eaa9bb344449d7ea0a314de254dc77449fa7aaa600f8cd9a24da65aade8c1ec6aa80c6e9a7bba5ca7

…sting 818dc74 Support serialization as another type without casting (Pieter Wuille) Pull request description: This adds a `READWRITEAS(type, obj)` macro which serializes `obj` as if it were converted to `const type&` when `const`, and to `type&` when non-`const`. No actual cast is involved, so this only works when this conversion can be done automatically. This makes it usable in serialization code that uses a single implementation for both serialization and deserializing, which doesn't know the constness of the object involved. This is a redo of bitcoin#12712, using a slightly different interface. Tree-SHA512: 262f0257284ff99b5ffaec9b997c194e221522ba35c3ac8eaa9bb344449d7ea0a314de254dc77449fa7aaa600f8cd9a24da65aade8c1ec6aa80c6e9a7bba5ca7

fanquake added the Refactoring label Mar 17, 2018

ryanofsky reviewed Mar 19, 2018

View reviewed changes

sipa closed this Mar 19, 2018

sipa mentioned this pull request Mar 20, 2018

Support serialization as another type without casting #12731

Merged

sipa mentioned this pull request May 17, 2018

Serialization improvements #10785

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Support serialization as another type without casting #12712

Support serialization as another type without casting #12712

Uh oh!

Conversation

sipa commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Mar 18, 2018

Uh oh!

meshcollider commented Mar 18, 2018

Uh oh!

donaloconnor commented Mar 18, 2018

Uh oh!

jimpo commented Mar 18, 2018

Uh oh!

sipa commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Mar 19, 2018

Uh oh!

ryanofsky commented Mar 19, 2018

Uh oh!

sipa commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Mar 19, 2018

Uh oh!

sipa commented Mar 19, 2018

Uh oh!

ryanofsky commented Mar 19, 2018

Uh oh!

jimpo commented Mar 19, 2018

Uh oh!

sipa commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sipa commented Mar 17, 2018 •

edited

Loading

sipa commented Mar 18, 2018 •

edited

Loading

ryanofsky left a comment •

edited

Loading

sipa commented Mar 19, 2018 •

edited

Loading