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.

Sign up for GitHub

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

Jump to bottom

os::mkdir and friends should maybe not take u32 #6085

Closed
brson opened this issue Apr 26, 2013 · 14 comments · Fixed by #13897
Closed

os::mkdir and friends should maybe not take u32 #6085

brson opened this issue Apr 26, 2013 · 14 comments · Fixed by #13897
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@brson
Copy link
Contributor

brson commented Apr 26, 2013

These are Rust functions, not C.
@brson brson mentioned this issue Apr 26, 2013
Closed
@sammykim
Copy link
Contributor

sammykim commented May 8, 2013

What should it take instead of c_int?

@brson
Copy link
Contributor Author

brson commented May 8, 2013

Here's some discussion from IRC

22:05 < strcat> brson: well, ideally not an enum
22:05 <@brson> yichoi: graydon could disagree though. I'm not sure what his vision for that module is and 
               he usually likes less abstraction than I
22:05 < strcat> we just need a flag type with | overloaded
22:05 < strcat> and some constants
22:05 < strcat> casting to an int to OR them would be annoying

@thestinger has some opinions

@yichoi
Copy link
Contributor

yichoi commented May 8, 2013

@thestinger refered
https://github.com/thestinger/rust-magic/commit/16a4fe0505f3c35cd4fa8fb051a5e6f396d2b2eb

@thestinger
Copy link
Contributor

I think structs are better for flags, because you're not restricted to the pre-defined variants. It wouldn't be safe to OR enum variants together unless you added a variant for each combination (match assumes it knows all of the variants, and so do the loads in the IR now).

It's much nicer to be able to do let base = FlagA | FlagC | FlagE and then call a function with base | FlagB than building up vectors.

@brson
Copy link
Contributor Author

brson commented May 8, 2013

We might want to use @nikomatsakis' EnumSet: nikomatsakis@75df9af#L10L-1

@emberian
Copy link
Member

Visiting for triage; still relevant. EnumSet still a viable solution.

@sammykim
Copy link
Contributor

sammykim commented Sep 3, 2013

It's still taking c_int and remains an issue.

@martindemello
Copy link
Contributor

I can take a shot at this. Is EnumSet still the way to go? And if so, would it need to be moved into libstd first?

@sanxiyn
Copy link
Member

sanxiyn commented Oct 21, 2013

#8054 moved EnumSet to libextra.

@brson brson mentioned this issue Nov 5, 2013
Closed
@richo
Copy link
Contributor

richo commented Mar 29, 2014

I'm interested in working on this. It looks like EnumSet has landed in core (Based on the hits for git grep EnumSet in the main repo).

Is there anything special I should know before I dive in? (Found this as a result of digging through issues tagged easy to play with)

@richo richo mentioned this issue Mar 29, 2014
Closed
@richo
Copy link
Contributor

richo commented Mar 29, 2014

I did some preliminary work to allow the use of EnumSet in std here: #13196

I'll tentatively start work on porting the permission flags, and then rebase when I found out the fate of that PR.

@erickt erickt mentioned this issue Apr 7, 2014
Open
@aturon
Copy link
Member

aturon commented Apr 25, 2014

Is this still an issue? This commit moved the functionality from os to std::io::fs and converted to u32 values with explicit flag constants. Do we want to replace this with something like the bitflags! macro?

/cc @brson @alexcrichton

@alexcrichton alexcrichton changed the title os::mkdir and friends should not take c_ints os::mkdir and friends should maybe not take u32 Apr 25, 2014
@alexcrichton
Copy link
Member

While they've been changed to u32, it's not clear (at least to me) that this is the correct type to use for these flags. One could imagine a bit set (using some primitive construct), but I'm not entirely convinced that it's the best representation either.

(updated title to be a bit more relevant)

@aturon aturon mentioned this issue May 2, 2014
Merged
aturon added a commit to aturon/rust that referenced this issue May 5, 2014
@aturon
Change std::io::FilePermission to a typesafe representation
8d1d7d9 
This patch changes `std::io::FilePermissions` from an exposed `u32`
representation to a typesafe representation (that only allows valid
flag combinations) using the `std::bitflags`, thus ensuring a greater
degree of safety on the Rust side.

Despite the change to the type, most code should continue to work
as-is, sincde the new type provides bit operations in the style of C
flags. To get at the underlying integer representation, use the `bits`
method; to (unsafely) convert to `FilePermissions`, use
`FilePermissions::from_bits`.

Closes rust-lang#6085.

[breaking-change]
bors added a commit that referenced this issue May 6, 2014
@bors
auto merge of #13897 : aturon/rust/issue-6085, r=bjz
cf6857b 
The `std::bitflags::bitflags!` macro did not provide support for
adding attributes to the generates structure, due to limitations in
the parser for macros. This patch works around the parser limitations
by requiring a `flags` keyword in the `bitflags!` invocations:

    bitflags!(
        #[deriving(Hash)]
        #[doc="Three flags"]
        flags Flags: u32 {
            FlagA       = 0x00000001,
            FlagB       = 0x00000010,
            FlagC       = 0x00000100
        }
    )

The intent of `std::bitflags` is to allow building type-safe wrappers
around C-style flags APIs. But in addition to construction these flags
from the Rust side, we need a way to convert them from the C
side. This patch adds a `from_bits` function, which is unsafe since
the bits in question may not represent a valid combination of flags.

Finally, this patch changes `std::io::FilePermissions` from an exposed
`u32` representation to a typesafe representation (that only allows valid
flag combinations) using the `std::bitflags`.

Closes #6085.
@richo
Copy link
Contributor

richo commented May 6, 2014

\o/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
Milestone
No milestone
10 participants
@martindemello @sanxiyn @alexcrichton @brson @richo @emberian @aturon @thestinger @yichoi @sammykim