Request for approval: absl::variant

[Peter Kasting]

As part of testing first-party absl support, and because it was already named as desirable, I wrote a CL to allow absl::variant, and converted one call site to use it: https://chromium-review.googlesource.com/c/chromium/src/+/2327456

I formally propose that we allow this feature.  Assuming no one objects, it can be the first allowed feature we announce first-party Abseil support with.

PK

[Daniel Murphy]

I'm a fan. Varints are used in IndexedDB (and LevelDBScopes) quite a lot, would be nice to migrate them.

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFA54sANKYRjvNfsLLU-aOzgHyS%2B_fxvSt%2BDPXmDn%2B3gog%40mail.gmail.com.

[Daniel Murphy]

LOL I thought you meant "VarInt" (as in variable-sized-integer) - nevermind!

[Anton Bikineev]

Peter, thanks for working on this. I'm very much in favor of having C++17-compatible variant in Chromium. It would help to better encapsulate various page states (swept, unswept, unswept_unfinalized, ...) in Oilpan. I think it would also be great to allow the same subset of Abseil in V8 as well after this is done in Chromium.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CA%2B4qT32K0yNwc0XMQF8P4woQRrzwpCQ4H%3DVo8n%2B-aNwyEFfROQ%40mail.gmail.com.

[Jan Wilken Dörrie]

Thanks, Peter!

I'm in favor of this as well, as I'd love using it within base::Value. A few questions:

* Right now we use CHECKs in base::Value to make sure you e.g. can't get a string from a Value that holds an int. For variants the obvious equivalent would be `absl::get<std::string>(my_variant)`, which throws an exception in case std::string is not the currently active type. Since we usually don't compile with exceptions turned on, this would result in program termination as well, correct?

* Should we allow absl::visit as well or limit its usage in some form? IIRC, alexclarke@ had some concerns regarding binary size for the version accepting multiple variants at the same time. See e.g. the discussion on https://crbug.com/1063997. 

Best,

Jan

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAFd1Zwd7UEpPpdrBqAcq9YB3gJZuXSMp4-FKh2-o3iEBGTpabA%40mail.gmail.com.

[Peter Kasting]

On Thu, Jul 30, 2020 at 10:51 PM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:

* Right now we use CHECKs in base::Value to make sure you e.g. can't get a string from a Value that holds an int. For variants the obvious equivalent would be `absl::get<std::string>(my_variant)`, which throws an exception in case std::string is not the currently active type. Since we usually don't compile with exceptions turned on, this would result in program termination as well, correct?

Attempting to throw in Chromium results in termination, yes.

Also note absl::holds_alternative<T>(v), which returns whether |v| contains an instance of type T.  So you could e.g. write

  CHECK(absl::holds_alternative<std::string>(v));

  return absl::get<std::string>(v);

IMO, if you want to enforce correct usage, this is better than "unconditional get() and let it throw if the caller was wrong".

 

* Should we allow absl::visit as well or limit its usage in some form? IIRC, alexclarke@ had some concerns regarding binary size for the version accepting multiple variants at the same time. See e.g. the discussion on https://crbug.com/1063997.

We should definitely allow it in some form, as visit() is important for a lot of variant use cases.  Passing multiple variants is certainly rarer than a single one; I don't know the exact binary size impact of doing so.  My inclination is always "clamp down as necessary" so I'm naturally inclined toward "allow it and see if there's really a problem".

PK

[Jan Wilken Dörrie]

On Fri, Jul 31, 2020, 08:25 Peter Kasting <pkas...@google.com> wrote:

On Thu, Jul 30, 2020 at 10:51 PM Jan Wilken Dörrie <jdoe...@chromium.org> wrote:

* Right now we use CHECKs in base::Value to make sure you e.g. can't get a string from a Value that holds an int. For variants the obvious equivalent would be `absl::get<std::string>(my_variant)`, which throws an exception in case std::string is not the currently active type. Since we usually don't compile with exceptions turned on, this would result in program termination as well, correct?

Attempting to throw in Chromium results in termination, yes.

Also note absl::holds_alternative<T>(v), which returns whether |v| contains an instance of type T.  So you could e.g. write

  CHECK(absl::holds_alternative<std::string>(v));

  return absl::get<std::string>(v);

IMO, if you want to enforce correct usage, this is better than "unconditional get() and let it throw if the caller was wrong".

Got it, thanks. And yes, I agree that explicit checks are likely better.

 

* Should we allow absl::visit as well or limit its usage in some form? IIRC, alexclarke@ had some concerns regarding binary size for the version accepting multiple variants at the same time. See e.g. the discussion on https://crbug.com/1063997.

We should definitely allow it in some form, as visit() is important for a lot of variant use cases.  Passing multiple variants is certainly rarer than a single one; I don't know the exact binary size impact of doing so.  My inclination is always "clamp down as necessary" so I'm naturally inclined toward "allow it

and see if there's really a problem".

Ack, SGTM.

PK

[Daniel Cheng]

Will we have tests that incorrect usage will cause program termination?

Daniel

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALB5StZdKjdzP-13PyJwV3%3D1ODbv4DKDK3iSuqiNyU89Vk%3DnYA%40mail.gmail.com.

[Jan Wilken Dörrie]

Are you talking about base::Value or absl::variant in general? We certainly can add DEATH tests to base::Value for this. Abseil's tests should take care of variant. Depending on the value of ABSL_HAVE_EXCEPTIONS these either expect a death or a throw. We should however enable those tests in Chromium and add them to our test suites. 

Best,

Jan

[Daniel Cheng]

Sorry, I was referring to tests for absl's variant. I would prefer if we had some test coverage that lives in Chrome and can't change out too unexpectedly from underneath us, but I might be a bit too cautious here.

Daniel

[Danil Chapovalov]

Created a change to enable those absl::variant tests:

https://chromium-review.googlesource.com/c/chromium/src/+/2332439

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CALB5StY%2B4itorcNbn-j3oUD6nft4w7xC6S96tYg9Z3uOU%2B75yQ%40mail.gmail.com.

[Jan Wilken Dörrie]

I'm happy to add death tests for base::Value, which then will also ensure that accessing a absl::variant in the wrong way will result in program termination. In addition to that we should still add the abseil tests to our Chromium test suites, mbonadei@ has started the work on this in https://crbug.com/1073011. There is also https://crbug.com/1096623 which explicitly tracks adding death tests for ABSL_OPTION_HARDENED.

Thanks danilchap@! Would you mind adding a few simple variant tests to https://source.chromium.org/chromium/chromium/src/+/master:third_party/abseil-cpp/absl_hardening_test.cc as well?

[Peter Kasting]

On Fri, Jul 31, 2020 at 1:20 AM Daniel Cheng <dch...@chromium.org> wrote:

Sorry, I was referring to tests for absl's variant. I would prefer if we had some test coverage that lives in Chrome and can't change out too unexpectedly from underneath us, but I might be a bit too cautious here.

I have CQ+2ed Danil's change that enables the absl hardening tests and adds some variant type mismatch death tests.

PK 

[Peter Kasting]

It's sounding like there's explicit support for this, no opposition, and some concerns about hardening that should be resolved soon.  Especially since variant has already been mentioned favorably in the past and has a bug requesting support, I tentatively consider this approved pending landing the hardening tests (and, of course, absl support in general).

Speak up soon if you have concerns :)

PK

[K. Moon]

Quick question: I assume absl::variant will eventually be replaced with std::variant? Will the testing story be similar for std::variant?

--

You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAAHOzFCnZUadgabCg-nmY3kaUXQNyo8sjMa27as1FzxEcE5ySA%40mail.gmail.com.

[Peter Kasting]

On Fri, Jul 31, 2020 at 9:12 AM K. Moon <km...@chromium.org> wrote:

Quick question: I assume absl::variant will eventually be replaced with std::variant? Will the testing story be similar for std::variant?

One of the approval conditions for absl was that we had a well-defined path to avoid losing hardening coverage.  In general, switching to std:: versions of anything we currently got from absl would probably be preceded by switching to them as backing the absl variants.  If they didn't offer the same checks, that would cause our existing tests to fail immediately, so we'd notice; and we would generally not proceed past that.

In this specific case, yes, trying to get<> an alternative a variant doesn't hold will throw std::bad_variant_access: https://en.cppreference.com/w/cpp/utility/variant/get

PK

[K. Moon]

Thanks; sounds good!