Skip to content

Restructure crate as core module #160

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

Merged
merged 1 commit into from
Sep 19, 2021
Merged

Restructure crate as core module #160

merged 1 commit into from
Sep 19, 2021

Conversation

workingjubilee
Copy link
Member

Aligns module with rust-lang/library/core, creating an... unusual
architecture that is easier to pull in as a module, as core itself can
have no dependencies (as we haven't built core yet).

@thomcc
Copy link
Member

thomcc commented Sep 19, 2021

One comment is that a downside of stdarch's, err, architecture, is that it has easily the worst IDE support of any part of the standard library (everything in core::arch::*:: shows up as unrecognized).

I'm not sure whether or not we should keep that in mind, but it would be nice if core::simd avoided this, somehow.

@calebzulawski
Copy link
Member

It looks like most of the changes are stylistic (since the imports got longer). One thing I don't understand--why make these changes at all? Can core not use core_simd as a crate dependency, and then do a simple pub use core_simd::*?

@thomcc
Copy link
Member

thomcc commented Sep 19, 2021

This code depends on core though, which would make it a circular dep, unless we do the work to make it #![no_core].

That said, I'm not sure I've ever seen a non-macro no_core crate.

@calebzulawski
Copy link
Member

Yeah, I suppose that makes sense.

@workingjubilee
Copy link
Member Author

I looked, for a long while, at alternatives.
I did not find a good one.

@workingjubilee
Copy link
Member Author

This code depends on core though, which would make it a circular dep, unless we do the work to make it #![no_core].

That said, I'm not sure I've ever seen a non-macro no_core crate.

This would break everything because then we would have to forge our own lang items, which would not match the ones in core.
Anyways, in the end, we can resolve this issue by simply closing out the subtree and thus this repo if we must.

@workingjubilee
Copy link
Member Author

One comment is that a downside of stdarch's, err, architecture, is that it has easily the worst IDE support of any part of the standard library (everything in core::arch::*:: shows up as unrecognized).

Also, is this due to the path or due to it being a submodule? Because this will be in-tree as a subtree for rust-lang/rust, not a submodule.

@workingjubilee
Copy link
Member Author

rust-lang/rust-analyzer#3898 It looks like there is an open bug for this? But
rust-lang/rust-analyzer#9963 at least in theory this is fixed for core::arch.
So this shouldn't actually be an issue.

@thomcc
Copy link
Member

thomcc commented Sep 19, 2021

But rust-lang/rust-analyzer#9963 at least in theory this is fixed for core::arch.
So this shouldn't actually be an issue.

I suspect this means core::arch resolves, but things inside it still don't. Or something like that, since I can confirm it's still an issue.

@thomcc
Copy link
Member

thomcc commented Sep 19, 2021

Also, is this due to the path or due to it being a submodule? Because this will be in-tree as a subtree for rust-lang/rust, not a submodule.

I believe it's because it's a #[path] that points outside the crate.

EDIT: This might mean if it's located inside https://github.com/rust-lang/rust/tree/master/library/core it would be fine.

@workingjubilee
Copy link
Member Author

I literally just tried

pub use core::arch::x86_64::{
    __m128, __m128i, _mm_add_ps, _mm_cvtps_epi32, _mm_load_ps, _mm_load_ss, _mm_max_ps, _mm_min_ps,
    _mm_mul_ps, _mm_set1_ps, _mm_setzero_ps, _mm_shuffle_ps, _mm_store_si128,
};

in a fresh crate and it seems to resolve fine.

@thomcc
Copy link
Member

thomcc commented Sep 19, 2021

Hm, maybe it's an issue with my setup and then it's not worth worrying about. I just wanted to avoid us having the same bad experience as core::arch due to developing out-of-tree.

@workingjubilee
Copy link
Member Author

At the moment I am using an Extremely Basic "Visual Studio Code + rust-analyzer plugin with barely any modifiers" setup so, nothing terrible exotic is likely responsible for a difference here.

I would be okay with nesting in theory but I am concerned that if we nest inside library/core/src/portable-simd/crates/core_simd/src we will risk running afoul of the Overly Long Path limitations on a Certain OS, and in general confusing the matter.

Aligns module with rust-lang/library/core, creating an... unusual
architecture that is easier to pull in as a module, as core itself can
have no dependencies (as we haven't built core yet).
@Lokathor
Copy link
Contributor

Last month I had bad enough problems with RA and a crate using core::arch that I just turned off RA because it was totally useless to constantly see fake errors. Might have been fixed since then though.

@workingjubilee
Copy link
Member Author

The patch in question was merged on Aug 20, so it is worth updating r-a and retrying.

@workingjubilee workingjubilee deleted the rearrange branch September 19, 2021 06:53
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.

4 participants