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

Tweak the _LIBCPP_INLINE_VISIBILITY patch so we can upstream it #222

Open
nox opened this issue Jan 7, 2020 · 6 comments
Open

Tweak the _LIBCPP_INLINE_VISIBILITY patch so we can upstream it #222

nox opened this issue Jan 7, 2020 · 6 comments

Comments

@nox
Copy link
Contributor

nox commented Jan 7, 2020

(@asajeffrey added this patch in #149.)

The configure code that sets this env variable in the moz.build stuff checks that the compiler is clang and the OS target is Android, as Alan said in the PR description.

I'm curious if the code could be tweaked to check the standard library rather than the compiler, which would allow us to upstream the patch more easily.

@jdm
Copy link
Member

jdm commented Jan 7, 2020

I think that's the wrong issue link?

@asajeffrey
Copy link
Member

Try #180

@asajeffrey
Copy link
Member

I'm not sure why we need any changes in what's being upstreamed, the patch just adds an env var for an already-existing configuration. All the stuff about Android, compilers, etc isn't in this patch.

@nox
Copy link
Contributor Author

nox commented Jan 7, 2020

I'm just suspecting that the check that targets clang does so because it actually wants to target libc++, which is the default libc++ of clang. Could be wrong though.

@asajeffrey
Copy link
Member

Could be, but that's probably an issue for upstream mozjs, not this crate?

@nox
Copy link
Contributor Author

nox commented Jan 8, 2020

Could be, but that's probably an issue for upstream mozjs, not this crate?

We are the one who wrote the patch, so I filed it here. I am not demanding immediate action, I had to file this somewhere to not forget it and I wasn't going to open a Bugzilla ticket.

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

No branches or pull requests

3 participants