Envoy: Convert absl::string_view to std::string_view and absl::optional to std::optional

[ggreenway]

Now that Envoy is using c++17, std::string_view is available.

To avoid a huge change to replace all uses in the code, absl::string_view will become an alias for std::string_view if we #define ABSL_USES_STD_STRING_VIEW (see https://github.com/abseil/abseil-cpp/blob/d39fe6cd6f5eb72dc741f17d3a143a6a5a56538a/absl/strings/string_view.h#L45).

New code should use std::string_view. Code, as it is modified, should be slowly converted to std::string_view. clang_tidy or check_format should stop allowing new uses of absl::string_view.

Same thing for absl::optional --> std::optional, using ABSL_USES_STD_OPTIONAL

[alyssawilk]

cc @yanavlasov @asraa @ahedberg for any import concerns.

[ggreenway]

It looks like absl will by default use the std version when compiled with c++17 (see https://github.com/abseil/abseil-cpp/blob/master/absl/base/options.h and https://github.com/abseil/abseil-cpp/blob/master/absl/base/config.h).

[asraa]

Thanks for the heads-up! Greg were you planning on taking this?
I'm in the mood to do format coding for some reason, so I'd love to take at least that portion :)

[ahedberg]

For anyone who doesn't have ABSL_USES_STD_STRING_VIEW enabled, this will cause problems of the following form:

• Envoy returns a std::string_view from some method
• Internal code wants to pass that std::string_view as an argument to another internal method
• That internal method accepts an absl::string_view, which is now a different type
• Now the call site to the internal method fails to compile

Google does not have ABSL_USES_STD_STRING_VIEW turned on; we can probably hack around this at import time, so I'm not that concerned if it goes forward for our sake. But it might be an issue for others.

(There is an analogous problem with optional and anything else that Abseil provides those compile-time switches for - variant, maybe?)

[stedsome]

@ggreenway @asraa I would love to take on this if this hasn't already been taken. Also, in response to Ashley's concern, Asra has suggested sending an email to envoy-announce to notify users of this potential problem. @ahedberg Is this going to be sufficient in addressing the issue?

[ahedberg]

That seems reasonable.

I've also been informed that my data was out of date, and Google does actually use ABSL_USES_STD_STRING_VIEW now. So we don't have any specific import concerns.

[stedsome]

Oh that sounds great.

[ggreenway]

@stedsome I think the only work we want to do now is to lint on new uses of absl::string_view/optional. I don't think we want an enormous change that rewrites from one type to the other. It just creates churn without much value, and makes it harder to read the code history in git.

[stedsome]

Yes, this is something I am aware of

[stedsome]

@lizan Lizan raises an interesting point that forcing the switch to std::string_view/optional will break
"Envoy Mobile on iOS 11 (which is its minimum iOS target), where std::string_view and std::optional doesn't exist in the runtime library. There's a compatibility layer in absl to handle C++17 for those platform:
https://github.com/abseil/abseil-cpp/blob/302b250e1d917ede77b5ff00a6fd9f28430f1563/absl/base/config.h#L460-L479"

[ggreenway]

Does the toolchain for iOS 11 support c++17 in general? If not, what's the overall plan for c++17 language features (not library additions, which can be added via absl)?

[stedsome]

I believe envoy-mobile now compiles and builds with toolchain for iOS13.5. So the language features should be supported without a problem. But envoy-mobile aims to be backward-compatible up to iOS10.0. If the binary were to run on these systems, we would probably encounter problems described by Lizan.

[ggreenway]

I wonder if envoy-mobile could add something to an always-included header like namespace std { using string_view = absl::string_view; using optional<T> = abls::optional<T>; }?

[stedsome]

@junr03

[junr03]

envoy-mobile aims to be backward-compatible up to iOS10.0

This is true. Although earlier this week we bumped the min requirement to iOS 11.0 https://github.com/lyft/envoy-mobile/pull/976

I wonder if envoy-mobile could add something to an always-included header

certainly open to that.

[lizan]

@ggreenway It's not the toolchain doesn't support C++17, it's the runtime library installed in iOS 11 doesn't have symbols needed for std::optional, so it fails at runtime. https://github.com/envoyproxy/envoy/pull/9654#issuecomment-580003294

From absl/base/config.h, we need workaround for std::any, std::variant and std::optional. std::string_view should be OK.

[junr03]

Alright I discussed with @rebello95 and @goaway and we think that if the Envoy project is ok with it we would like to keep the Envoy codebase using absl types until Envoy Mobile drops support for iOS 11. We don't see a large disadvantage for doing so, and it would prevent hairy type aliases in the Envoy Mobile codebase. Envoy Mobile will support iOS 11 through at least the end of the year, but if we want to expedite that process our team can start a conversation after iOS 14 lands this fall.

In order to ameliorate reviewer cognitive load and prevent any issues we can have a linter that conversely to #12388 checks that the absl types are being used until Envoy is ready to migrate.

Relatedly, and in response to:

I don't think we want an enormous change that rewrites from one type to the other.

My 2 cents is that once we are ready to start using std, I wonder if we do really want to keep both usages for a while, seems to me that asking for someone to look a little harder at git history is not that bad a price to pay for homogeneity.

wdyt @ggreenway @lizan @stedsome?

[goaway]

Just to chime in, while I the see reasoning behind some of the arguments against this approach in a general sense, I do think it's worth considering that iOS isn't just a random platform - it's one of the most widely-deployed operating systems in the world. And for now, this is what's required for Envoy (not just Envoy mobile) to be able to run on recent versions of iOS (granted, probably most cases of that are Envoy Mobile).

That to me seems like potentially enough of a justification to hold off on migrating from absl (or introducing new calls with std) until some set horizon (which could be tracked in an issue).

[mattklein123]

I don't see any reason not to just stick with absl for these types. There is no really difference to the project or dev productivity. Please do the linter!

[ggreenway]

I don't see any reason not to just stick with absl for these types. There is no really difference to the project or dev productivity. Please do the linter!

I agree.

Even if it meant whole-sale no c++17 support, I'd be ok with delaying that until platforms we want/need to support have the toolchain/runtime support required. (But fortunately, it sounds like only a few types in the standard library, which have drop-in absl equivalents, are missing).

[lizan]

Agreed, I don't see any strong reason to do replace them immediately if it poses problem on iOS/macOS.

In order to ameliorate reviewer cognitive load and prevent any issues we can have a linter that conversely to #12388 checks that the absl types are being used until Envoy is ready to migrate.

+1, we should do this for the three types (any, optional, variant).

[junr03]

+1, we should do this for the three types (any, optional, variant).

@stedsome let me know if you'd like to take that given you authored #12388. Otherwise, I can take it.

[stedsome]

@junr03 I am happy to do the work. What about std::string_view v.s. absl::string_view? It seems that string_view differs from the other three types (any, optional, variant) in that it is functional on iOS11 according to this. https://github.com/abseil/abseil-cpp/blob/302b250e1d917ede77b5ff00a6fd9f28430f1563/absl/base/config.h#L460-L479

[junr03]

I am happy to do the work.

Awesome, thanks! I can create an issue for the eventual migration of those three types.

What about std::string_view v.s. absl::string_view

Yep, it is not necessarily blocked by iOS/MacOS.

My personal opinion from above is:

Relatedly, and in response to:

I don't think we want an enormous change that rewrites from one type to the other.

My 2 cents is that once we are ready to start using std, I wonder if we do really want to keep both usages for a while, seems to me that asking for someone to look a little harder at git history is not that bad a price to pay for homogeneity.

So I would commit to only one usage, either std because it is not blocked right now, or absl and defer migration until we tackle the three types currently blocked. But I know @ggreenway or @lizan might have a different opinion.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted". Thank you for your contributions.