Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make EncodableValue a thin std::variant wrapper #19983

Merged
merged 4 commits into from Aug 4, 2020

Conversation

stuartmorgan
Copy link
Contributor

Description

Instead of a hand-rolled discriminated union (originally used to avoid a C++17
dependency, which is no longer an issue), implement EncodableValue as a
std::variant. Rather than simply changing the internals, this makes EncodableValue
a minimal std::variant subclass with only a handful of added method, replacing
the old IsFoo/FooValue APIs with the standard std::holds_alternative/std::get,
so that plugin code will use a standard-based API rather than a Flutter-specific
API for wrapped values.

This is a breaking change for Windows and GLFW plugins. In the short
term USE_LEGACY_ENCODABLE_VALUE can be set in builds to use the old
version, to separate rolling from updating.

Related Issues

Fixes flutter/flutter#61970

Tests

I added the following tests: Updated all existing tests, added a few for specific pitfalls that the subclass avoids, such as char*->bool conversion.

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the contributor guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the C++, Objective-C, Java style guides for the engine.
  • I read the tree hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation.
  • All existing and new tests are passing.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

This is a breaking change for Windows and GLFW plugins. In the short
term USE_LEGACY_ENCODABLE_VALUE can be set in builds to use the old
version, to separate rolling from updating.

Fixes flutter/flutter#61970
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me what the stance of these wrappers is around exceptions. Right now, the user can expect std::bad_variant_access which they may find surprising. Everything else is a nit.

//
// The order/indexes of the variant types is part of the API surface, and is
// guaranteed not to change.
class EncodableValue : public std::variant<std::monostate,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variant definition is long and repeated. How about just calling it EncodableValueVariant up top and using it once?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth on that; my concern is that if it's declared with a name someone might use it directly, probably on accident (e.g., picking EncodableValueVariant instead of EncodableValue in IDE autocompletion). I felt like duplicating a value that I don't expect to change (except one more time when I make this extensible for custom StandardCodec types) was better than adding something undesired to the global namespace.

I guess the other option would be to declare it in a namespace internal {} or something along those lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in an internal namespace.

using super::super;
using super::operator=;

explicit EncodableValue() : super() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary because the monostate is already part of the variant. = default should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, will change to = default. (I was surprised I needed to declare it at all, vs. having it come in through the using, actually).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


// Returns true if the value is null. Convenience wrapper since unlike the
// other types, std::monostate uses aren't self-documenting.
bool IsNull() const { return index() == 0; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That the index of the monotstate is 0 is not immediately obvious (though I realize you have documented it as being stable and part of the API). How about just std:holds_alternative<std::monostate>(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I meant to change this back. I started with that more obvious implementation, but tried this while I was trying to figure out why IsNull was giving me weird results in a unit test (which turned out eventually to be the pointer->bool->EncodableValue(bool) implicit conversion chain with an accidental call-with-pointer when this was freestanding.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

const EncodableList& ListValue() const {
return std::get<EncodableList>(*this);
}
EncodableList& ListValue() { return std::get<EncodableList>(*this); }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize you said it was programmer error if the types don't match and while this is certainly true in Google code, in non-Google standard C++, I believe an exception will be thrown instead. Are these wrappers meant to be exception aware?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to say that it's an exception. (I'd still argue that's a programming error, but better to be precise)

//
// It is a programming error to call these method if the value doesn't
// contain the correct collection type.
const EncodableList& ListValue() const {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these really more convenient? Seems like unnecessary ceremony around std::variant whose uses are well known and documented already. Your call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's specifically for the chained use called out in the comment. The issue is that the more you chain, the more the type extraction and the indexing operator are separated.

I did originally intend not to have any of these methods, but encountered a couple of places where I was doing things like in the comment, and the new version felt significantly less readable. There's an argument that maybe using temporary variables is a better solution, but it becomes more verbose quickly.

Although those cases were mostly in unit tests, so maybe it's not something that will realistically happen very often. I'll look again at the FDE plugins and see if I ever actually used this in real code, and if not I can remove them under the principle that we can add them later if there is real-world demand, but removing them later is hard.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was only one non-unit-test case where I was really using it, and it was easy to just add a temporary, so I've removed these.

@chinmaygarde
Copy link
Member

Can we land this or are you planning on tinkering with this further?

@stuartmorgan
Copy link
Contributor Author

I'm building and running unit tests on the version with your comments addressed right now.

Copy link
Contributor Author

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed comments, and added back old versions of StandardCodec implementations behind ifdefs, since it compiled on the client side and thus subject to the same ifdef.

//
// The order/indexes of the variant types is part of the API surface, and is
// guaranteed not to change.
class EncodableValue : public std::variant<std::monostate,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in an internal namespace.

using super::super;
using super::operator=;

explicit EncodableValue() : super() {}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


// Returns true if the value is null. Convenience wrapper since unlike the
// other types, std::monostate uses aren't self-documenting.
bool IsNull() const { return index() == 0; }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

//
// It is a programming error to call these method if the value doesn't
// contain the correct collection type.
const EncodableList& ListValue() const {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was only one non-unit-test case where I was really using it, and it was easy to just add a temporary, so I've removed these.

@flar flar removed their request for review July 31, 2020 21:48
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still lgtm.

@stuartmorgan
Copy link
Contributor Author

I triggered re-runs of all of the "failed" (actually cancelled) jobs, and all five were green. Just waiting on the tree to be green to land this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants