Request to allow first-party dependency on third_party/abseil-cpp/absl/status

[Xiaohan Wang (王消寒)]

I am requesting to allow first-party dependency on third_party/abseil-cpp/absl/status.

Use case:

In media/ code, we'd like to be able to return detailed reasons of error for media components to facilitate metrics and dev tools reporting, as well as better implementation of MediaError.message. The current plan is to use media::Status we recently added, which shares a lot of design with absl::Status. Since we allow to use abseil now, it makes sense to use absl::Status directly to avoid duplication.

Best,

Xiaohan

[Peter Kasting]

It looks like the Status API uses string_view and Cord in functions you'd want to call.  Would it make sense for you to continue to have media::Status but turn it into a thin wrapper around an absl::Status that mostly translates between Chromium and absl types for these sorts of cases?  That would avoid proliferating banned type usage, and IMO fits within the spirit of the "use banned types as minimally as possible when interfacing with third-party code" rule.

PK

[Xiaohan Wang (王消寒)]

Indeed I do plan to still keep media::Status which wraps absl::Status, because we may need some additional features for media code, and it's longer to include third_party/abseil-cpp/absl/status.h :)

How does this work with the "allowed first party dependency": It's allowed but must be wrapped to avoid proliferating other types that are still banned?

[Peter Kasting]

On Mon, Aug 3, 2020 at 10:28 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Indeed I do plan to still keep media::Status which wraps absl::Status, because we may need some additional features for media code, and it's longer to include third_party/abseil-cpp/absl/status.h :)

How does this work with the "allowed first party dependency": It's allowed but must be wrapped to avoid proliferating other types that are still banned?

Well, we're still in uncharted territory with this stuff :).

I think it's possible for callers to use absl::Status directly without calling the APIs that use still-banned types, in which case we'd ideally allow them to Just Use It.  But we wouldn't want to have the other types proliferate.

I wonder if this is an argument for e.g. base/util/absl_wrappers or something so we can put consumer-agnostic wrapper types/wrapper APIs around absl items in cases like this, and then consumers can either further extend those or use them directly safely.  What do other cxx@ folks (especially base/ OWNERS) think?

PK 

[Xiaohan Wang (王消寒)]

Any update/plan on this one?

[Peter Kasting]

On Mon, Aug 10, 2020 at 9:13 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Any update/plan on this one?

There's a CL in review to add conversions between base::StringPiece and absl::string_view.  That might make wrapping this easier.

I don't have a problem with Status if it's wrapped and people use the wrapper type.  You might separately want to start a thread for Cord if you want more direct use of absl::Status.

PK

[Xiaohan Wang (王消寒)]

We'll keep using media::Status as a wrapper type wrapping absl::Status internally.

How does the process work? Is there any additional process we need to go through? Or can we treat this thread as the approval?

[Peter Kasting]

On Mon, Aug 10, 2020 at 5:01 PM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

We'll keep using media::Status as a wrapper type wrapping absl::Status internally.

How does the process work? Is there any additional process we need to go through? Or can we treat this thread as the approval?

I'm a bit uncomfortable being a sole voice here -- I'd like to at least give other cxx@ folks until the end of this week to explicitly weigh in.

PK 

[Jan Wilken Dörrie]

My 2 cents: Having media::Status as a thin wrapper around absl::Status SGTM. 

media::Status should not use Abseil string types in its public API, but rather use base::StringPiece instead of absl::string_view and base::StringPiece or std::string instead of absl::Cord. Internally it should convert between those types, making use of abseil_string_conversions.h to convert to and from absl::string_view.

A wider adoption of absl::Status will require figuring out a story for absl::Cord, i.e. either allow or ban. In the case of a ban we should add appropriate conversions to abseil_string_conversions.h.

Best,

Jan

--
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/CAAHOzFCwrZR%2BLp6p7jNtpfMmZby2Jx_9YkTfQX%2BqU1KwG3-Gcg%40mail.gmail.com.

[Peter Kasting]

On Mon, Aug 3, 2020 at 8:53 AM Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

I am requesting to allow first-party dependency on third_party/abseil-cpp/absl/status.

Approved for use inside a wrapper type.  Use abseil_string_conversions.h to convert to and from absl::string_view so the wrapper can expose StringPiece.  Use absl::Cord directly as minimally necessary to interface; do not expose in the wrapper type API.  (Cord itself has a somewhat broad API, so Just Allowing it will be a bit challenging, but we could discuss if it turns out that's a good future direction.)

I have not written a CL to update the banned/approved types list.  Will let you do that as part of implementation here.

PK

[Xiaohan Wang (王消寒)]

Thanks! Will send a CL to update the banned/approved types list.

--

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/CAAHOzFBZ-wTZ4LAR9M_aXO0-vJ4_YuLgU%2BuTPkLgZFTe0iTBKw%40mail.gmail.com.