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

Replace default_stat in std::path with a Default impl for libc::stat #9652

Closed

Conversation

MicahChalmer
Copy link
Contributor

Instead of the internal default_stat function in std::path, make the stat structure implement Default and have the path code use that. Closes #9537. Alternative to #9637.

@@ -392,6 +421,32 @@ pub mod types {
st_blocks: blkcnt_t,
st_pad5: [c_long, ..14],
}
impl Default for stat{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space

@lilyball
Copy link
Contributor

lilyball commented Oct 1, 2013

This is going to conflict with my Path rewrite (#9655).

@alexcrichton
Copy link
Member

As with a similar pull request recently, I feel like this suffers from the "making code less readable but more abstract" problem. It's really nice to call static methods on traits on type parameters, but it's not so nice to call static methods on traits directly.

If I saw let stat = Default::default() I wouldn't get nearly as much meaning out of it as let stat = stat::default_stat() or something like that. I may be one of just a few who feel this way, though, but I personally prefer to avoid invoking static methods directly on traits.

@lilyball
Copy link
Contributor

lilyball commented Oct 1, 2013

@alexcrichton The problem is the stat struct is provided by libc. The default_stat() method is part of std::path::stat, which is really kind of sucky. It's not the right place to get a zero-filled libc::stat struct. So either add non-extern functions to libc, which is odd, or implement Default, which seems alright to me.

@alexcrichton
Copy link
Member

Oh I agree that there should be some method of getting a zero'd out stat struct, I just feel like isn't the best way of going about doing this. It's still the problem of where the first question you ask is "how do I get a stat struct?" and when the answer is "Default::default" you start questioning your morals.

@MicahChalmer
Copy link
Contributor Author

I think that the use of default to fill in missing fields is such a common pattern that it deserves a feature like the Default trait. But I agree that having to write Default::default() sucks. It seems straightforward to improve: all that would be needed is a standalone std::default::default function just like std::num::zero has: fn default<T: Default>() -> T { Default::default() }. You could then use default() where needed without having to mention the trait. (And in places where it's not obvious or inferable you'd have default::<stat>().)

Given that the usual answer to optional/named parameters is "use a struct," and given that there is special treatment for Default already (the "deriving" attribute) I also think both the Default trait and the aforementioned standalone default function should be added to the prelude.

Would that make it feel less immoral?

In any case, here I'm just trying to make it possible to use stat without writing separate code for each platform, while staying with the convention that is established in the code as it exists...

@huonw
Copy link
Member

huonw commented Oct 1, 2013

It seems straightforward to improve: all that would be needed is a standalone std::default::default function just like std::num::zero has: fn default<T: Default>() -> T { Default::default() }.

This is working around a language issue that should be resolved at the "source" (making calling static methods on traits more "sensible", how ever that is done) rather than continually papering over the symptoms by defining standalone functions.

@alexcrichton
Copy link
Member

In my opinion, I think this should be closed in favor of #9637, #9537 would be wontfix, and #9637 would use re-exporting to expose the default stat function.

This would probably need more discussion for whether that's the best route to take, however.

@alexcrichton
Copy link
Member

I'm going to close this an #9637 because I don't think that this is the right way to go about doing this. The stat syscall itself should not be getting used, and we don't want to encourage use of it. Instead the stat portions of rt::io::file should be getting used, and it's currently a legacy bug that the Path apis are still using these old stat structures (see #9958). Once we remove the consumers of those stat structures, then there's no need for this in libstd.

If it does end up being a large use case that external code needs to call libc::stat, then we can reconsider. For now it's still possible with a malloc and size_of, and it's certainly not the best interface but it gets the job done.

@MicahChalmer MicahChalmer deleted the stat_impls_default branch October 21, 2013 02:17
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 20, 2022
[`zero_prefixed_literal`] Do not advise to use octal form if not possible

fix rust-lang/rust-clippy#9651

changelog: [`zero_prefixed_literal`] Do not advise to use octal form if not possible
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.

libc::stat should derive Default
4 participants