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

Make machine types different from int/uint, etc. #2187

Closed
brson opened this issue Apr 11, 2012 · 24 comments
Closed

Make machine types different from int/uint, etc. #2187

brson opened this issue Apr 11, 2012 · 24 comments
Labels
A-typesystem Area: The type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Milestone

Comments

@brson
Copy link
Contributor

brson commented Apr 11, 2012

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.

@nikomatsakis
Copy link
Contributor

+1

@graydon
Copy link
Contributor

graydon commented Apr 11, 2012

Agreed. So long as you can cast I think this should be ok.

@nikomatsakis
Copy link
Contributor

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?

@brson
Copy link
Contributor Author

brson commented Apr 11, 2012

I think this will force the float modules to use a bunch of wrapper functions.

@ghost ghost assigned brson Apr 12, 2012
@boggle
Copy link
Contributor

boggle commented Apr 14, 2012

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.

@brson
Copy link
Contributor Author

brson commented Apr 14, 2012

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:

// Use one template module to specify in a single file the implementation
// of functions for multiple types

mod mystd {

    // The template on type T is specified in float-template.rs
    #[path = "float-template"]
    mod float {
        // The type of the float
        import inst::T;

        // Define T as float
        #[path = "inst_float.rs"]
        mod inst;
    }

    // Use the same template
    #[path = "float-template"]
    mod f64 {

        import inst::T;

        // Define T as f64
        #[path = "inst_f64.rs"]
        mod inst;
    }

    #[path = "float-template"]
    mod f32 {
        import inst::T;

        #[path = "inst_f32.rs"]
        mod inst;
    }

}

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

@boggle
Copy link
Contributor

boggle commented Apr 14, 2012

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.

@nikomatsakis
Copy link
Contributor

Woah, @brson, I do not quite understand what's going on there, but it looks cool. What does #[path] do?

@nikomatsakis
Copy link
Contributor

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 #include, so in fact we do have a kind of macro today (and macros let us build up simple traits, too).

@boggle
Copy link
Contributor

boggle commented Apr 14, 2012

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).

@brson
Copy link
Contributor Author

brson commented Apr 14, 2012

@boggle, yes, the float mod would have to have two versions, one with #[cfg(target_arch = "x86_64")] that uses f64-template and another for x86.

@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 #[inline(always)]. I think the issue is mostly one of code duplication.

@brson
Copy link
Contributor Author

brson commented Apr 14, 2012

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?

@brson
Copy link
Contributor Author

brson commented Apr 14, 2012

Oh, #[path] just specifies the name of the file/directory to use as the implementation of a module.

@boggle
Copy link
Contributor

boggle commented Apr 15, 2012

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 src/libmath/math.rc

boggle@fbe857f

@brson
Copy link
Contributor Author

brson commented Apr 15, 2012

@boggle I did it as well for int/uint in #2210. It was about a 200 line reduction.

@brson
Copy link
Contributor Author

brson commented Apr 15, 2012

@boggle I think you can simplify things more with const casts, like ten can be generically defined as const ten: T = 10f as T

@boggle
Copy link
Contributor

boggle commented Apr 15, 2012

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.

@brson
Copy link
Contributor Author

brson commented Apr 15, 2012

@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?

@boggle
Copy link
Contributor

boggle commented Apr 15, 2012

@brson 10 as T works nicely.

@brson
Copy link
Contributor Author

brson commented Apr 15, 2012

We have a dependency on libstdc++ and that depends on libm.

@boggle
Copy link
Contributor

boggle commented Apr 15, 2012

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?

@brson
Copy link
Contributor Author

brson commented Apr 15, 2012

@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.

@boggle
Copy link
Contributor

boggle commented Apr 15, 2012

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.

@boggle
Copy link
Contributor

boggle commented Apr 15, 2012

Continuing this in #2198

@eholk eholk closed this as completed Jun 5, 2012
@brson brson unassigned eholk Jun 16, 2014
celinval added a commit to celinval/rust-dev that referenced this issue Jun 4, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-typesystem Area: The type system E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Development

No branches or pull requests

5 participants