-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Make machine types different from int/uint, etc. #2187
Comments
+1 |
Agreed. So long as you can cast I think this should be ok. |
we probably want to address the reason that this change was introduced in the first place, which if I recall had something to do with sharing code in the various float modules? "boggle" on IRC was working on it, I think? |
I think this will force the |
Yes, if I remember correctly the plan was to add a pragma to enable this behavior per module or function. I really would not want the float module to have to use wrapper functions for performance reasons; an alternative could be to support typecasting of function prototypes on export as a means of explicit unification. |
I think there is a way to avoid duplicating code here by using a single 'template' module with multiple types. We would arrange the modules like:
It gets more complicated when you take into account that, in the real library, you would have two implementations of the template, one for 32-bit types and one for 64-bit. There is a full test case in 611061b |
Interesting idea, this could work, I did not know about #[path]. Available annotations like that need more documentation. I have two questions though: Can I use #[cfg] on mod float in this scenario to switch depending on the architecture? Does that work with native modules (f32, f64 right now directly rename lib math calls to proper rust calls via #[link_name])? While I find this ok for core::float, in general I wonder wether this is not a bit too much overhead for someone who just wants to write library bindings in a similar situation and needs float <--> fXY equality. |
Woah, @brson, I do not quite understand what's going on there, but it looks cool. What does |
Brian's stuff got me to thinking. To be honest, I don't really understand what the problem with the floating point modules was precisely (I guess I have to go look at core::float?), but I am sure we can solve it through some combination of ifaces/impls, macros, and (eventuaIly) traits. The annoying part is that only the first one exists today, or so we thought: but Brian's module trick essentially gives us C's |
Look at core::float. The gist is this: Each function in C lib math exists in one version per datatype (f32, f64, ...). I use native modules that in a first step rename these to identical function names. After this point all that is wanted is to export the f64 ones as f64, the f32 ones as f32, and whichever happens to be identical to float as float. Any suggestions welcome but please don't introduce unnecessary wrappers etc, I'd really like to keep this fast. I see traits here more as a means of building a number tower later on (maybe a "native" impl that directly links agains a C lib could help). |
@boggle, yes, the @nikomatsakis I think this technique does things that traits and impls can't. I arrived at it after trying to think of a way to use impls without a bunch of wrappers. @boggle Wrappers should not be a performance concern here since we can mark them with |
I think this technique should allow anything that can be put into a module to be parametrized over... anything else that can be put into a module? |
Oh, |
I took this for a spin and it looks like it is working. The only thing I still need is explicit casting from f32, f64 to float in the const definitions. Now we need to decide if this should become a separate crate or not (cf. #2198). Here's the code, doesn't build via make, look in |
@boggle I think you can simplify things more with const casts, like |
I was a bit worried if this would work out with native libs but it did fine and everything is much cleaner this way. Now I want to know where to put it, core or separate crate. |
@boggle I think that modularity is always a good thing so we should go ahead and separate it, especially since you've already done the heavy lifting, but we should also reexport them from core for simplicity. It doesn't seem clear to me that libm is too heavy for core. Will we be fine on android with libm in core? |
@brson 10 as T works nicely. |
We have a dependency on libstdc++ and that depends on libm. |
Hmm ok, that is a fair point. Regarding reexporting, won't this create a cyclic dependency between core and lib math? How should I go about integrating it into the build? |
@boggle I didn't realize libmath had a dependency on core, assumed it went the other way. If it's not possible to remove the core dependency from libmath then I suppose it should just stay in core. Maybe though some of the custom functions in libmath (like string conversion) can be defined in core, leaving libmath mainly to wrap libm. |
I did a quick check, this seems to be exclusively about to_str. The impl of to_str for str /could/ go to lib math but then there is still exfmt which calls to_str directly. I'll try to separate things such that most of lib math becomes an independent crate but the to_str and perhaps from_str stuff goes to core for the time being. I think it may actually be a good idea to further strim down core and put all the to_str related bits in a separate place (lib std, lib format), some systems don't have printing facilities in their "core" library. |
Continuing this in #2198 |
I tried applying model-checking/kani#2983 fix, however, this would require user to import `__kani_workaround_core_assert`. To fix that, I moved the definition to be under `kani` crate. I replaced the existing fixme test. Initially I didn't check we had one, and I created a second one which is simpler (no cargo needed) but that also includes other cases. Resolves rust-lang#2187
Right now i64 unifies with int on 64-bit platforms, etc, and it's a source of errors like in d65df5d. I believe they should be different types.
The text was updated successfully, but these errors were encountered: