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

Incorrect const-ness on CString::new argument #18117

Closed
canndrew opened this issue Oct 17, 2014 · 3 comments
Closed

Incorrect const-ness on CString::new argument #18117

canndrew opened this issue Oct 17, 2014 · 3 comments

Comments

@canndrew
Copy link
Contributor

The signature of CString::new is CString::new(buf: *const i8, owns_buffer: bool). The problem with this is that if the new CString does own the buffer it's going to free it. In which case buf should be a *mut i8 not a *const i8. However making buf: *mut i8 doesn't account for the case where the CString doesn't own the buffer and the buffer is immutable.

To fix this CString::new should be split into two functions:

unsafe fn CString::new(buf: *const i8) -> CString;
unsafe fn CString::new_owned(buf: *mut i8) -> CString;
@nodakai
Copy link
Contributor

nodakai commented Oct 17, 2014

Because CString basically doesn't write anything into the memory chunk pointed by buf, I think taking *const c_char as an argument is OK. And dereferencing a raw pointer is always unsafe regardless whether it's mut or const, after all.

However I also felt unconfortable with CString::new() and want to split it into two functions. Choosing a correct one from either CString::new(buf, true) or CString::new(buf, false) is a mental burden (bor both of reading and writing.) In fact, we rarely want to parametrize the 2nd argument; for example, all 25 invocations of CString::new() in rust/src used literal true or false.

My idea is naming them CString::as_owner(buf) (for the true case) and CString::as_view(buf) (for the false case) but I don't know how they sound to native English speakers. It may be unclear they are so-called constructors.

@canndrew
Copy link
Contributor Author

@nodokai I had already written the patch when I read your comment so I went with new and new_owned.

@alexcrichton
Copy link
Member

This was since resolved in #20507

lnicola pushed a commit to lnicola/rust that referenced this issue Sep 25, 2024
fix: Always cache macro expansions' root node in Semantics

Previously some expansions were not cached, but were cached in the expansion cache, which caused panics when later queries tried to lookup the node from the expansion cache.

Fixes rust-lang#18089.
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

No branches or pull requests

3 participants