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 ctype compat macros. NFC #20960

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Dec 20, 2023

The code here was unused because of the fact that we already define _LIBCPP_HAS_MUSL_LIBC in libcxx/include/__config_site which means that _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE is
defined by libcxx/include/__config which in turn mean that the first branch of this if/else
tree is the that is being used.

This change should really have been part of #17022 which was when we started defining
_LIBCPP_HAS_MUSL_LIBC.

libcxx has direct support for `_NEWLIB_VERSION` in `__locale` so just
use that.  This libcxx support was added in http://reviews.llvm.org/D7888
and landed in emscripten in emscripten-core#13154.
@sbc100 sbc100 requested review from dschuff and kripken December 20, 2023 17:58
@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 20, 2023

Finally catching up with a change @jfbastien made 8 years ago!

@sbc100 sbc100 marked this pull request as ready for review December 20, 2023 17:58
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Where do we define _NEWLIB_VERSION?

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 20, 2023

Sorry, brain fart, this has nothing to do with newlib.

The change is still valid but for a different reason. Its because we define _LIBCPP_HAS_MUSL_LIBC which in turn defines _LIBCPP_PROVIDES_DEFAULT_RUNE_TABLE. I will update the PR description.

@sbc100 sbc100 requested a review from kripken December 20, 2023 19:32
@dschuff
Copy link
Member

dschuff commented Dec 20, 2023

Was this a downstream localmod or do we need to also upstream the __locale change?

@sbc100
Copy link
Collaborator Author

sbc100 commented Dec 20, 2023

Was this a downstream localmod or do we need to also upstream the __locale change?

The original change was local change that made its way upstream already, so is not longer a local mod.

This change will become a new local change that, like all local changes, should be upstreamed at some point. My philosophy for this is to periodically looks at the deltas from upstream and push any new changes at that point.

@sbc100 sbc100 merged commit 4f9f1e1 into emscripten-core:main Dec 20, 2023
@sbc100 sbc100 deleted the remove_ctype_hacks_2 branch December 20, 2023 21:00
aheejin added a commit to aheejin/llvm-project that referenced this pull request May 22, 2024
This upstreams more recent, mostly EH changes from libcxx and libcxxabi:
- `__cxa_init_primary_exception`-related changes made when updating to
  LLVM 18.1.2 (emscripten-core/emscripten#21638)
- Removes ctype macros
  (emscripten-core/emscripten#20960)
- Guard destructor changes with `__wasm__`
  (emscripten-core/emscripten#21974)
aheejin added a commit to llvm/llvm-project that referenced this pull request May 22, 2024
This upstreams more recent, mostly EH changes from libcxx and libcxxabi:
- `__cxa_init_primary_exception`-related changes made when updating to
LLVM 18.1.2 (emscripten-core/emscripten#21638)
- Removes ctype macros
(emscripten-core/emscripten#20960)
- Guard destructor changes with `__wasm__`
(emscripten-core/emscripten#21974)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants