Default-allowing Abseil features

[Peter Kasting]

Per our C++ feature policy, we should default-allow the following TBD Abseil features that have been in the codebase for more than two years, unless we have a good reason to ban:

• bind_front [tbd]
• Containers [tbd]
• Container utilities [tbd]
• Random [tbd]
• StatusOr [tbd]
• String Formatting [tbd]
• Strings Library [tbd]
• Synchronization [tbd]
• Time library [tbd]

Here's my thoughts on these:

bind_front: Ban, recommend base::Bind instead

Containers: Allow, without guidance on when to use (guidance, and switching some of our usage to these, hopefully to come someday)?

Container utilities: Ban, recommend base::ranges::algorithm.h instead

Random: ? Need to see how this overlaps with base/rand_util.h

StatusOr: Ban, recommend base::expected instead

String Formatting: ? Can we replace base::StringPrintf() with this?

Strings Library: ? Can we replace various base/strings things with these?

Synchronization: ? Can we replace anything in base/synchronization here?  If not, ban

Time library: Ban, recommend base/time/ instead

PK

[Daniel Cheng]

Containers: it would be nice to have some benchmarks. But I think things like btree_map and flat_hash_map are much better default choices than what we currently have. The main issue is sorting out the big mess around hashing types.

Random should be banned, for the same rationale as the C++ standard random number engines.

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/CAAHOzFCV8%3DW58EPxr-9jA86Zs%3DFa2Y5nRRaDcmCZySmNzg%2Bf5g%40mail.gmail.com.

[Peter Kasting]

On Fri, Sep 9, 2022 at 11:00 AM Daniel Cheng <dch...@chromium.org> wrote:

Random should be banned, for the same rationale as the C++ standard random number engines.

I read back through our discussion thread on <random>, and it seemed like the majority of our complaints were things the Abseil library addresses, like poor algorithms and seeding.  In particular, people were asking for the Randen prng, which is what this uses.

I assume the remaining dealbreaker is whether things are cryptographically secure, which Abseil is not.  I wonder if we should at least allow the Abseil distribution functions with the base bit generator, and replacement of base::InsecureRandomGenerator?

PK

[Daniel Cheng]

Yes, this is a dealbreaker. If we want to use it inside the implementation of base::InsecureRandomGenerator, that's an option, but usage of base::InsecureRandomGenerator intentionally requires adding a friend. We shouldn't allow absl's directly.

Daniel

 

PK

[Kyle Charbonneau]

String Formatting: ? Can we replace base::StringPrintf() with this?

Given all the work happening to support C++20, are there any compelling reasons to adopt absl::StrFormat() over std::format()? Both provide type safe string formatting so I would learn towards adopting the standard library. If C++20 adoption is far enough away there is the fmt library that implements a std::format() compatible API we could use in the interim.

Strings Library: ? Can we replace various base/strings things with these?

These utilities are closely coupled with the banned absl::string_view type. In particular they rely on absl::string_view as a return value. I think we should ban this as a result to avoid having absl::string_view leak into chromium code. After both chromium and absl support C++20 with std::string_view we could revisit this and likely replace some of base string utilities with absl string utilities.

--

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/CAF3XrKroV01BweTSAUqa0zqy5AMi%3DjDVX8i9CV2w-dCfMHA58A%40mail.gmail.com.

[K. Moon]

There's an internal C++ style thread about std::format vs. absl::StrFormat() from a couple years back; I'm not sure where this discussion would land now, but one issue raised with std::format is that it uses exceptions for error handling.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAERSRDARhybF7qZmny5%3Dzb_%3DY1o3TzESFUQOTw1A7cwhLZz_eg%40mail.gmail.com.

[Danil Chapovalov]

On Monday, September 12, 2022 at 4:49:26 PM UTC+2 kyle...@chromium.org wrote

Strings Library: ? Can we replace various base/strings things with these?

These utilities are closely coupled with the banned absl::string_view type. In particular they rely on absl::string_view as a return value. I think we should ban this as a result to avoid having absl::string_view leak into chromium code. After both chromium and absl support C++20 with std::string_view we could revisit this and likely replace some of base string utilities with absl string utilities.

Note that it is possible and easy to configure abseil to force absl::string_view to be an alias of the std::string_view

see ABSL_OPTION_USE_STD_STRING_VIEW in 

https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/base/options.h;l=135;drc=6c9d33d6a27dfb75bed218615240acf6278dc332

 

[Peter Kasting]

On Fri, Sep 9, 2022 at 10:54 AM Peter Kasting <pkas...@google.com> wrote:

Strings Library: ? Can we replace various base/strings things with these?

I've posted in the internal thread about StrFormat vs. std::format.  For now, my weak inclination is to say we should try to auto-port from base::StringPrintf() to absl::StrFormat() (easier than auto-porting to std::format, so even if the latter is allowed, it will probably be some time coming) and allow this, but I'm not sure whether we want to "ban until someone wants to actually tackle that migration plan"?

 

Synchronization: ? Can we replace anything in base/synchronization here?  If not, ban

I glanced at this briefly and didn't come to any conclusions.  It does look like Abseil has support for some of base's synchronization objects here, but with different names/APIs.  It's worth investigating this as any low-level performance wins we can get are likely to be meaningful, but I think someone actually has to do the investigation.  If we must come to a short-term conclusion it might be "banned for lack of demand but we welcome investigation"?

PK

[Kyle Charbonneau]

String Formatting: ? Can we replace base::StringPrintf() with this?

 

There's an internal C++ style thread about std::format vs. absl::StrFormat() from a couple years back; I'm not sure where this discussion would land now, but one issue raised with std::format is that it uses exceptions for error handling.

 

I've posted in the internal thread about StrFormat vs. std::format.  For now, my weak inclination is to say we should try to auto-port from base::StringPrintf() to absl::StrFormat() (easier than auto-porting to std::format, so even if the latter is allowed, it will probably be some time coming) and allow this, but I'm not sure whether we want to "ban until someone wants to actually tackle that migration plan"?

So reading through the internal thread absl::StrFormat() has some advantages over std::format().

1. Similar formatting specifier syntax to StringPrintf(). For example base::StringPrintf("Hello %s", name) is absl::StrFormat("Hello %s", name) instead of std::format("Hello {}", name).
2. Compile time validation of format strings. std::format() gets this in C++23. 
3. Runtime validation for format strings that aren't known at compile time.
4. No exceptions for error handling.

That does seem like a compelling reason to allow absl::StrFormat(). I agree either allow absl::StrFormat() or ban but only until someone has a chance to come up with a migration plan. 

Strings Library: ? Can we replace various base/strings things with these?

These utilities are closely coupled with the banned absl::string_view type. In particular they rely on absl::string_view as a return value. I think we should ban this as a result to avoid having absl::string_view leak into chromium code. After both chromium and absl support C++20 with std::string_view we could revisit this and likely replace some of base string utilities with absl string utilities.

Note that it is possible and easy to configure abseil to force absl::string_view to be an alias of the std::string_view

see ABSL_OPTION_USE_STD_STRING_VIEW in 

https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/base/options.h;l=135;drc=6c9d33d6a27dfb75bed218615240acf6278dc332

Right, I forgot std::string_view is available in C++17 we just haven't migrated to it from base::StringPiece yet. There is a cxx thread and an old bug related to that. So maybe ban absl string library until std::string_view is allowed and then revisit?

--

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/CAAHOzFA4sc%3D9Cf2i_0hRv_MQecwUHWhwqanygPRw6bd5mMyLSA%40mail.gmail.com.

[Peter Kasting]

On Tue, Sep 13, 2022 at 5:38 PM Kyle Charbonneau <kyle...@chromium.org> wrote:

1. Compile time validation of format strings. std::format() gets this in C++23. 

I thought this was (retroactively) part of C++20, via the DR on this.  Or were there further C++23-only changes?

1. No exceptions for error handling.

In fairness, I'm not sure this one matters much, unless it throws for errors that we actually want to handle at runtime.  Crashing the program for things like ill-formed format strings seems OK (if less friendly than catching them at compile time).

Strings Library: ? Can we replace various base/strings things with these?

These utilities are closely coupled with the banned absl::string_view type. In particular they rely on absl::string_view as a return value. I think we should ban this as a result to avoid having absl::string_view leak into chromium code. After both chromium and absl support C++20 with std::string_view we could revisit this and likely replace some of base string utilities with absl string utilities.

Note that it is possible and easy to configure abseil to force absl::string_view to be an alias of the std::string_view

see ABSL_OPTION_USE_STD_STRING_VIEW in 

https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/base/options.h;l=135;drc=6c9d33d6a27dfb75bed218615240acf6278dc332

Right, I forgot std::string_view is available in C++17 we just haven't migrated to it from base::StringPiece yet. There is a cxx thread and an old bug related to that. So maybe ban absl string library until std::string_view is allowed and then revisit?

Yeah, once crbug.com/1335422 is fixed we can start thinking about this migration.

PK

[Kyle Charbonneau]

1. Compile time validation of format strings. std::format() gets this in C++23. 

I thought this was (retroactively) part of C++20, via the DR on this.  Or were there further C++23-only changes?

Yep, you're right, P2216R3 was accepted as a defect report for C++20. Scratch that reason then.

[Peter Kasting]

Continuing to try to drive this to a conclusion.  If you have feedback, please reply this week.  If I hear little else I'll implement the below.

bind_front: Ban, recommend base::Bind instead

Containers: Allow, without guidance on when to use.  File bug (against dcheng?) to benchmark, write guidance, and ideally migrate existing usage.

Container utilities: Ban, recommend base::ranges::algorithm.h instead

Random: Ban, recommend base/rand_util.h instead

StatusOr: Ban, recommend base::expected instead

String Formatting: Ban for now.  File bug (against me) to migrate base::StringPrintf() to absl::StrFormat().
Strings Library: Ban for now.  File bug (against me) to re-evaluate when we use std:string_view.
Synchronization: Ban.  File bug (unowned) to benchmark absl vs. base vs. std implementations.

Time library: Ban, recommend base/time/ instead

PK

[dan...@chromium.org]

lgtm

--

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/CAAHOzFAgfTAs-u_JPDGYpobspH86y-Anjrg90AcLCZbq%3D0NLsg%40mail.gmail.com.

[Nico Weber]

At the moment, the absl component build story on Windows isn't great (https://crbug.com/1345505, "Get rid of absl def files"). I'd feel better if we could resolve that before going all-in on absl.

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

[Peter Kasting]

I'm familiar with that bug, but I'm not sure how it affects this discussion. As long as we use any absl, we pay the penalties of absl/libc++/build flags changes being messy, and using more seems like it shouldn't cost anything extra. Additionally, the recommendation on this thread is to go ahead and ban almost everything remaining anyway.

Can you give more details about what costs you're worried about incurring?

PK

[Nico Weber]

Fixing that bug will require adding export statements to absl. I imagine that'll be easier to start rolling out while the set of used APIs is smaller (?)

[Peter Kasting]

Hmm. Mirko, can you comment on the effects of allowing the absl containers on the ease of rolling out a fix for this bug in the future?

PK

[Mirko Bonadei]

Yes, the smaller the usage the easier it is, but on the other hand if these types are going to be used anyway, we will have to ABSL_EXPORT them anyway when we roll Abseil every week so it is just a bit more work that gets done earlier.

On this topic, I have to prioritize a bit this month but I am going to give it another push in the next few days.

[Peter Kasting]

On Wed, Sep 28, 2022 at 8:04 AM Mirko Bonadei <mbon...@webrtc.org> wrote:

Yes, the smaller the usage the easier it is, but on the other hand if these types are going to be used anyway, we will have to ABSL_EXPORT them anyway when we roll Abseil every week so it is just a bit more work that gets done earlier.

On this topic, I have to prioritize a bit this month but I am going to give it another push in the next few days.

Thanks.  Given that, it seems like it should be OK to resolve the status of these containers as "allowed" now.

PK 

[Peter Kasting]

I plan to implement the above changes this week.

PK 

[Victor Vasiliev]

I don't understand the motivation for banning absl::StatusOr while allowing absl::Status.

Is the idea that the users are supposed to use base::expected<T, absl::Status> instead?  The third-party code that uses Status would almost inevitably use StatusOr for those scenarios, and given that the purpose of the status API is to introduce a uniform way of propagating errors, I am not sure adding an incompatible Chromium-specific way of doing StatusOr will make things better.

Thanks,

  Victor.

--

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/CAAHOzFCo9kooKVB3fFeyh_%3DFUHK3EquXw9fy8epB%2BS0A815eYw%40mail.gmail.com.

[Daniel Cheng]

I think we generally allow exceptions for code that needs to interoperate with third-party libraries. We generally try to convert if we need to pass the values more widely in Chrome though.

Daniel

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

[Victor Vasiliev]

I understand that part; I am trying to understand the advantage of using expected<T, Status>, when Status is explicitly designed to be used with StatusOr and that's the idiomatic usage that's documented and people are familiar with.

[Daniel Cheng]

There was some previous discussion about https://groups.google.com/a/chromium.org/g/cxx/c/ucsnOkZhS2c/ here.

I'm not really sure why absl::Status is allowed but absl::StatusOr is not. absl::Status doesn't seem widely used anyway, except to interop with tflite and such.

Daniel

[David Benjamin]

FWIW, I would also agree that StatusOr probably makes more sense with Status. In particular, the Status type contains OkStatus() in it. expected<T, E> expects that E only contains errors. Using expected<T, Status> would allow callers to inadvertently return unexpected(OkStatus()) and likely cause issues at boundaries. StatusOr is aware of this quirk of Status and includes this check here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/abseil-cpp/absl/status/internal/statusor_internal.h;drc=95f3e04e349bf59cdf13c4f4d81748a2b392dcaf;l=216

I do think expected<T, E> is a better design (the runtime check only exists due to a slightly too expansive type), but to use it with absl::Status cleanly would require an absl::Error type that contains the non-OkStatus values of absl::Status. So I think we should treat them as a pair and not try to mix and match. (I.e. either import both for 1p usages, or say both should only be used for interop with 3p libraries and that 1p usage should prefer some other pattern, perhaps expected with an error-only error type.)

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/CAF3XrKpsth-duUKipae9P8R2V0v8aiq%3D%2B_Y6%3DRPRzPUQEXvYEQ%40mail.gmail.com.

[Peter Kasting]

On Fri, Oct 21, 2022 at 10:23 AM Victor Vasiliev <vas...@google.com> wrote:

I understand that part; I am trying to understand the advantage of using expected<T, Status>, when Status is explicitly designed to be used with StatusOr and that's the idiomatic usage that's documented and people are familiar with.

Honestly I wonder if we should ban absl::Status (except as needed for interop) to be consistent with StatusOr.

I wouldn't expect any Chromium code to use Status alone, StatusOr, or expected<T, Status>. I would expect Chromium code to use more restrictive, more local error enums.

PK 

[Alex Cooper]

FWIW, absl::Status used to be listed as "Allowed" but then had some caveats that it was only allowed in a wrapper type, which made me kind of curious why it wasn't listed as banned since that was more closely related to "Banned" than "Allowed" in my mind. It looks like the cleanup removed some of the nuance that was previously listed about this: https://chromium-review.googlesource.com/c/chromium/src/+/3939489

--

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/CAAHOzFB%2BBjG%2B6j6-sMpm_Wf1%2B_%2BnDPfsu2khtFGzW_22JHmfTg%40mail.gmail.com.