feat: Add a feature that allows usage on no_std targets #242
Conversation
There was a problem hiding this comment.
Okay, so: I definitely think we would like to work in no-std environments; this is something we've talked about, but never been motivated to actually approach.
That said, I'm not sure if we want to assume the solution is always musl? I really don't know what our options are, though, so I'd be curious if @dfrg or @raphlinus have any opinions.
I've reviewed this as-is; my main issue currently is that the trait is sort of half-baked, since we define it in both cases but it's only ever used if std is missing; if this is the approach we take I think we should simplify this trait to only exist if std is not active.
examples/circle.rs
Outdated
| println!("</svg>"); | ||
| println!("</body>"); | ||
| println!("</html>"); | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
Can we do something like just adding #![cfg(feature = "std")] to the top of the file? (Where possible, I prefer solutions that minimize diff size)
examples/ellipse.rs
Outdated
| println!("</svg>"); | ||
| println!("</body>"); | ||
| println!("</html>"); | ||
| #[cfg(feature = "std")] |
src/common.rs
Outdated
|
|
||
| impl FloatFuncs for f32 { | ||
| $(fn $name(self $(,$arg: $arg_ty)*) -> $ret { | ||
| #[cfg(feature = "std")] |
There was a problem hiding this comment.
slightly pedantic, but if we do want to use this trait in both cases (which would require us to call the trait methods directly, or give them names that do not shadow the std names) I would do this the other way around: if libm is set, always use libm, if libm is not set and std is set use std. (Because perhaps for some reason you want to use musl even if std is available? Tell me if I'm wrong 💁)
| @@ -4,6 +4,91 @@ | |||
|
|
|||
| use arrayvec::ArrayVec; | |||
|
|
|||
| /// Defines a trait that chooses between libstd or libm implementations of float methods. | |||
There was a problem hiding this comment.
so... is this actually necessary in the case of std? If std is present then these methods will already exist on our types, and method resolution will work, so it feels like we could get away with only defining this trait if the musl feature is active?
(the fact that this trait does nothing if std is active is supported by the need to add #[allow(unused_code)] on the import)
|
Thanks for the review!
Just to clarify, the
Good point! I'll go back and fix it like this when I have the chance. |
|
@cmyr What is the status of reviewing this PR? |
Okay cool, I hadn't understood that on my first pass. This can get rebased to bring in the more recent clippy fixes, and then I'm going to solicit a second opinion; modulo that I'm happy to merge! |
|
Prior art: #172 |
|
I’m so conflicted on this one. I’d really like to see no-std support in kurbo and this PR does what essentially amounts to the best that can be done given the constraints. My primary concern is that pulling in a dependency that transitively references kurbo without std will still make use of the suboptimal math functions even if std exists in the full dependency graph. There is an issue in libm that suggests making all symbols weak which would solve this problem but that hasn’t been done yet. That said, since we can override this by adding an explicit kurbo dep with std, I’m also inclined to suggest merging. +1 |
notgull
force-pushed
the
no_std_attempt_2
branch
from
b39d119 to
7cdbf09
Compare
|
I've rebased on the latest |
There was a problem hiding this comment.
I think chad makes an interesting point, but there is a clear workaround available if this problem crops up, so I think there is clear value in landing this.
edit: I've made one last suggestion below (https://github.com/linebender/kurbo/pull/242/files#r1140642582), happy to merge when we figure that out.
Thanks @notgull!
This PR adds features
std(enabled by default) andlibmthat allow this crate to be used onno_stdtargets.