--
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/CACSOxh4KVimnFfZHGJa-m8rBGR0nyyecOPK7iKQVjgZ%2BG%3DbKUg%40mail.gmail.com.
Any feelpinions regarding adoption of RE2 avec Abseil? FWIW, https://github.com/google/re2/blob/abseil/BUILD#L88-L97 is what's currently used. Migrating callers from re2::StringPiece to absl::string_view would probably require the most effort, I think?
Sorry, I'm not familiar enough with re2 to understand the question sufficiently. Can you give more detail? It looks like you're proposing changing Chromium to pull from upstream's "abseil" branch rather than "master", but I don't know what that really connotes in terms of features, API, and support/stability.
When you say "use", do you mean in implementation or APIs, or both? Implementation uses are no problem; API changes are probably more of one.
That said, I don't think the small differences between base::StringPiece and absl::string_view should block us from adopting 3rd party libraries making use of the latter. The two types are very similar, have the exact same memory layout and converting between them is cheap and easy. Thus I am in favor of (sufficiently constrained) implicit constructors and conversion operators, as this should unblock the adoption of otherwise very useful Abseil libraries. However, we should ensure that we limit their usage as much as possible, and ideally only use them on 3rd party API boundaries. I expressed a similar opinion in the initial allow Abseil thread.
That said, I don't think the small differences between base::StringPiece and absl::string_view should block us from adopting 3rd party libraries making use of the latter. The two types are very similar, have the exact same memory layout and converting between them is cheap and easy. Thus I am in favor of (sufficiently constrained) implicit constructors and conversion operators, as this should unblock the adoption of otherwise very useful Abseil libraries. However, we should ensure that we limit their usage as much as possible, and ideally only use them on 3rd party API boundaries. I expressed a similar opinion in the initial allow Abseil thread.
On Wed, Aug 5, 2020 at 1:03 AM Jan Wilken Dörrie <jdoe...@google.com> wrote:That said, I don't think the small differences between base::StringPiece and absl::string_view should block us from adopting 3rd party libraries making use of the latter. The two types are very similar, have the exact same memory layout and converting between them is cheap and easy. Thus I am in favor of (sufficiently constrained) implicit constructors and conversion operators, as this should unblock the adoption of otherwise very useful Abseil libraries. However, we should ensure that we limit their usage as much as possible, and ideally only use them on 3rd party API boundaries. I expressed a similar opinion in the initial allow Abseil thread.I'm not sure how we would do the conversion implicitly, yet limit its use to the boundary of third-party code (although if only third-party code can use string_view, then we sort of get that anyway).It looks like today re2::StringPiece <-> StringPiece happens manually if at all (see https://source.chromium.org/chromium/chromium/src/+/master:extensions/browser/api/declarative_net_request/regex_rules_matcher.cc;l=40;drc=bfc918abc39f0349f4993b2ae47429ce05e7619f for a sorta-similar case). Perhaps we could start by:
- Adding some conversion routines between base::StringPiece and absl::string_view, e.g. as new //base/strings/ APIs (StringPieceToStringView() and StringViewToStringPiece()?). This lets us allow that specific file to access string_view.
- Changing our re2 deps to pull from the abseil branch and fixing the callers that use re2::StringPiece directly to use base::StringPiece by means of the above wrapper APIs (there don't look to be very many)
On Thu, Aug 6, 2020 at 5:16 PM Peter Kasting <pkas...@google.com> wrote:On Wed, Aug 5, 2020 at 1:03 AM Jan Wilken Dörrie <jdoe...@google.com> wrote:That said, I don't think the small differences between base::StringPiece and absl::string_view should block us from adopting 3rd party libraries making use of the latter. The two types are very similar, have the exact same memory layout and converting between them is cheap and easy. Thus I am in favor of (sufficiently constrained) implicit constructors and conversion operators, as this should unblock the adoption of otherwise very useful Abseil libraries. However, we should ensure that we limit their usage as much as possible, and ideally only use them on 3rd party API boundaries. I expressed a similar opinion in the initial allow Abseil thread.I'm not sure how we would do the conversion implicitly, yet limit its use to the boundary of third-party code (although if only third-party code can use string_view, then we sort of get that anyway).It looks like today re2::StringPiece <-> StringPiece happens manually if at all (see https://source.chromium.org/chromium/chromium/src/+/master:extensions/browser/api/declarative_net_request/regex_rules_matcher.cc;l=40;drc=bfc918abc39f0349f4993b2ae47429ce05e7619f for a sorta-similar case). Perhaps we could start by:
- Adding some conversion routines between base::StringPiece and absl::string_view, e.g. as new //base/strings/ APIs (StringPieceToStringView() and StringViewToStringPiece()?). This lets us allow that specific file to access string_view.
I fail to see how this would be different than allowing implicit conversions between absl::string_view and base::StringPiece. Couldn't we only allow //base/strings/string_piece.h to #include "third_party/abseil-cpp/absl/strings/string_view.h", and forbid other references to absl::string_view via a PRESUBMIT check?
- Changing our re2 deps to pull from the abseil branch and fixing the callers that use re2::StringPiece directly to use base::StringPiece by means of the above wrapper APIs (there don't look to be very many)
Things will be a bit trickier for APIs that don't consume a single string_view, but rather an array of them, e.g. RE2::Match. Here you'd either have to create a temporary array and copy them over one-by-one, or resort to reinterpret_cast, which likely borders on UB. Implicit conversions won't solve this either (can't convert between Container<T> and Container<U>, even if T and U are convertible), so we'd have to extend the range of utility functions here.
On Thu, Aug 6, 2020 at 9:09 AM 'Jan Wilken Dörrie' via cxx <c...@chromium.org> wrote:On Thu, Aug 6, 2020 at 5:16 PM Peter Kasting <pkas...@google.com> wrote:On Wed, Aug 5, 2020 at 1:03 AM Jan Wilken Dörrie <jdoe...@google.com> wrote:That said, I don't think the small differences between base::StringPiece and absl::string_view should block us from adopting 3rd party libraries making use of the latter. The two types are very similar, have the exact same memory layout and converting between them is cheap and easy. Thus I am in favor of (sufficiently constrained) implicit constructors and conversion operators, as this should unblock the adoption of otherwise very useful Abseil libraries. However, we should ensure that we limit their usage as much as possible, and ideally only use them on 3rd party API boundaries. I expressed a similar opinion in the initial allow Abseil thread.I'm not sure how we would do the conversion implicitly, yet limit its use to the boundary of third-party code (although if only third-party code can use string_view, then we sort of get that anyway).It looks like today re2::StringPiece <-> StringPiece happens manually if at all (see https://source.chromium.org/chromium/chromium/src/+/master:extensions/browser/api/declarative_net_request/regex_rules_matcher.cc;l=40;drc=bfc918abc39f0349f4993b2ae47429ce05e7619f for a sorta-similar case). Perhaps we could start by:
- Adding some conversion routines between base::StringPiece and absl::string_view, e.g. as new //base/strings/ APIs (StringPieceToStringView() and StringViewToStringPiece()?). This lets us allow that specific file to access string_view.
I fail to see how this would be different than allowing implicit conversions between absl::string_view and base::StringPiece. Couldn't we only allow //base/strings/string_piece.h to #include "third_party/abseil-cpp/absl/strings/string_view.h", and forbid other references to absl::string_view via a PRESUBMIT check?My concern is twofold:(1) Since #includes are transitive, if string_piece.h #includes string_view.h, now everyone who #includes string_piece.h -- including indirectly -- has string_view in scope. We can probably PRESUBMIT-check against these, but I'd rather the compile fail. If instead some_string_piece_conversion_file.h #includes string_view.h, the only places bringing that in scope will be individual .cc files that want to call the conversions. It's not viral.(2) As an author or reviewer, an explicit type conversion is obvious; an implicit one is not. It's much easier to accidentally write or commit code using a banned type with implicit conversions.I don't think the difference between the proposals is enormous, but I think there's a small win here.
- Changing our re2 deps to pull from the abseil branch and fixing the callers that use re2::StringPiece directly to use base::StringPiece by means of the above wrapper APIs (there don't look to be very many)
Things will be a bit trickier for APIs that don't consume a single string_view, but rather an array of them, e.g. RE2::Match. Here you'd either have to create a temporary array and copy them over one-by-one, or resort to reinterpret_cast, which likely borders on UB. Implicit conversions won't solve this either (can't convert between Container<T> and Container<U>, even if T and U are convertible), so we'd have to extend the range of utility functions here.Correct, and this applies no matter what we decide on the previous question. OTOH, this is already an issue today since we don't use re2::StringPiece except for boundary code here, so in my view "this is no worse".
PK
I think we should follow your suggestion of adding a dedicated header to //base/strings (or potentially //base/util/... like previously mentioned), containing the following conversions:* absl::string_view to base::StringPiece* base::StringPiece to absl::string_view* base::span<const absl::string_view> to std::vector<base::StringPiece>* base::span<const base::StringPiece> to std::vector<absl::string_view>The last two are intended to cover the array and vector cases, which should be the most popular outside of the singular conversions. This then should be sufficient to unblock the adoption of re2/abseil and other Abseil features that rely on absl::string_view in their public API.
On Thu, Aug 6, 2020 at 1:08 PM Jan Wilken Dörrie <jdoe...@google.com> wrote:I think we should follow your suggestion of adding a dedicated header to //base/strings (or potentially //base/util/... like previously mentioned), containing the following conversions:* absl::string_view to base::StringPiece* base::StringPiece to absl::string_view* base::span<const absl::string_view> to std::vector<base::StringPiece>* base::span<const base::StringPiece> to std::vector<absl::string_view>The last two are intended to cover the array and vector cases, which should be the most popular outside of the singular conversions. This then should be sufficient to unblock the adoption of re2/abseil and other Abseil features that rely on absl::string_view in their public API.Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1113880 , since I think we want this in general regardless of this specific library.
Let's assume we have that. Then it comes back to the original question here. Do we want to pull the abseil re2 instead of the main one? Is this even a cxx@ question at this point, since it's not really about allowing something new?
PK
I do want to circle back to the original proposal here. It sounds like the primary goal is to make re2 perform better (faster/less memory). Is that correct, or is there more to the story?
OK. FIled https://bugs.chromium.org/p/chromium/issues/detail?id=1114173 to track this question, then. Interested parties can star/comment there.Thanks for the suggestion, Paul!