Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove custom optional implementation. #3286

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

rnlahaye
Copy link
Contributor

It is no longer needed as of #3279.

@pleroy
Copy link
Member

pleroy commented Jan 19, 2022

[Automated message from GitHub Pull Request Builder] Answer "ok to test" to trigger testing of this PR.

@rnlahaye
Copy link
Contributor Author

ok to test

@@ -121,8 +121,6 @@ endif
ifeq ($(UNAME_S),Darwin)
INCLUDES += \
-I$(DEP_DIR)compatibility/filesystem \
Copy link
Member

Choose a reason for hiding this comment

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

Can we get rid of this one too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filesystem requires 10.15, so no.

Clang 12 is willing to compile filesystem while targeting 10.12, but I haven't actually tried running the resulting binary on a pre-Catalina system; I suspect there might be runtime errors in that case.

Copy link
Member

@eggrobin eggrobin Jan 20, 2022

Choose a reason for hiding this comment

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

Yeah now that I think of it there is a note to that effect in the script (and a citation of Cicero for good measure):

# NOTE(egg): We need Clang 8, and therefore we use Xcode 11. The libc++ provided by
# Xcode 11 has the <filesystem> header, however its symbols are marked unavailable
# unless macosx-version-min is at least 10.15 (Catalina). Since we do not want to
# require that version yet (quousque tandem?), we cannot use the libc++ filesystem.

@pleroy pleroy merged commit ee45562 into mockingbirdnest:master Jan 20, 2022
@pleroy pleroy added the LGTM label Jan 20, 2022
@rnlahaye rnlahaye deleted the optional branch January 20, 2022 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants