Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Use vendored version of <experimental/optional> #13049

Merged
merged 5 commits into from
Jan 21, 2019
Merged

Use vendored version of <experimental/optional> #13049

merged 5 commits into from
Jan 21, 2019

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Oct 8, 2018

<experimental/optional> is not available anymore in newer versions of libc++, according to libc++'s TS deprecation policy: https://libcxx.llvm.org/TS_deprecation.html. This unblocks support for the Android NDK 18: #12972 (comment)

@kkaefer kkaefer added build Core The cross-platform C++ core, aka mbgl labels Oct 8, 2018
@kkaefer kkaefer force-pushed the optional branch 2 times, most recently from 37314c1 to 62c0d62 Compare October 8, 2018 13:17
@kkaefer
Copy link
Member Author

kkaefer commented Oct 8, 2018

/cc @mollymerp: it looks like there's a crash in the pattern implementation: the line-pattern/property-function test is crashing in debug mode because the optional here is empty but gets dereferenced:

}

constexpr T const& value() const& {
return initialized() ? contained_val() : (throw bad_optional_access("bad optional access"), contained_val());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the GNU implementation it's calling

  // XXX Does not belong here.
  inline void
  __throw_bad_optional_access(const char* __s)
  { _GLIBCXX_THROW_OR_ABORT(bad_optional_access(__s)); }

that optionally aborts instead of throwing, can we also keep this in the vendored implementation ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pozdnyakov do you know of a (non-STL-specific) version that does this? I've checked both https://github.com/martinmoene/optional-lite and https://github.com/akrzemi1/Optional, and they both don't have accommodations for disabling exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried https://github.com/martinmoene/optional-lite in https://github.com/mapbox/mapbox-gl-native/compare/optional2, and it's significantly smaller for Android, but larger for iOS.

https://github.com/TartanLlama/optional doesn't have a nothrow version either.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@springmeyer
Copy link
Contributor

@kkaefer when do you anticipate this landing? Asking since this fix will be needed to upgrade to clang++ 7 on OS X too (mapbox/vtshaver#21), which now includes the latest libc++ that drops <experimental/optional>

@tobrun
Copy link
Member

tobrun commented Jan 9, 2019

is there an eta for landing this PR? This would help moving forward to upgrade NDK to 18 in #12972

@kkaefer kkaefer force-pushed the optional branch 4 times, most recently from 725deae to 54b562a Compare January 21, 2019 10:53
@kkaefer kkaefer merged commit a4b814d into master Jan 21, 2019
@kkaefer kkaefer deleted the optional branch January 21, 2019 16:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Core The cross-platform C++ core, aka mbgl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants