Skip to content

Update vec.rs #27068

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

Closed
wants to merge 1 commit into from
Closed

Update vec.rs #27068

wants to merge 1 commit into from

Conversation

rick68
Copy link
Contributor

@rick68 rick68 commented Jul 16, 2015

Check the capacity of Vec<T> that is more than or equal to length.

Check the capacity of `Vec<T>` that is more than or equal to length.
@rust-highfive
Copy link
Contributor

r? @huonw

(rust_highfive has picked a reviewer for you, use r? to override)

@brson
Copy link
Contributor

brson commented Jul 16, 2015

This looks good to me - I think this invariant is strongly-implied by the existing docs. But it is not stated explicitly, do you mind adding a bullet to the 'Unsafety' section stating this invariant?

@alexcrichton does this debug assert seem appropriate to you? Do we have other debug asserts enforcing invariants like this in std?

@brson
Copy link
Contributor

brson commented Jul 16, 2015

Hm, note that nobody will ever hit this because std is never compiled with debug asserts. This mostly serves as code docs.

@pnkfelix
Copy link
Member

@brson std is never compiled with debug asserts? Even with a configure --enable-debug build? That is interesting if true.

@brson
Copy link
Contributor

brson commented Jul 16, 2015

@pnkfelix oh, 'never' is too strong of course. More like users never see debug asserts in std.

@rick68
Copy link
Contributor Author

rick68 commented Jul 16, 2015

@brson I just wanted this to be working in debug mode for the users, wondering if it won't work (libcollections is not compiled with support of debug_assert! macro)

@alexcrichton
Copy link
Member

The from_raw_parts function is such a low-level function for manipulating vectors (it's basically exposing the representation) I don't think we should place many asserts around it. It's nice to have a clear connection between this function and the creation of a vector with literally zero extra overhead or checks. It's clearly very unsafe but I don't think that this assertion will buy us much in the long run.

@Gankra
Copy link
Contributor

Gankra commented Jul 16, 2015

This also has exception-safety implications. e.g. if someone doesn't expect this to panic, but it suddenly does in debug, that's not great... although on the other hand it's basically UB to hit this assert anyway.

Also as has been noted, this will literally only run if someone builds their own custom compiler with debug assertions turned on. Using a stable/beta/nightly build this assertion will be stripped.

@brson
Copy link
Contributor

brson commented Jul 16, 2015

@rick68 Unfortunately, because 'std' itself is compiled in release mode, even when users of std compile their projects in debug mode they will not get debug assertions that originate in std.

@rick68
Copy link
Contributor Author

rick68 commented Jul 17, 2015

Thinking about generics, collections::vec::Vec can not just be used in std. If I want to use collections::vec::Vec to develop my own custom containers, this is able to access memory out of range.

But this is the risk what I have to take if I want to use unsafe methods.

@Gankra
Copy link
Contributor

Gankra commented Jul 17, 2015

@rick68 yes most collections are generally unsafe.

Anyway, it seems like we agree this should be closed.

@Gankra Gankra closed this Jul 17, 2015
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.

7 participants