Make RustString an extern type to avoid improper_ctypes warnings#132549
Conversation
rustbot
added
S-waiting-on-review
|
I also want to rename |
|
See also #116963 for general dissatisfaction with |
|
I'm not 100% confident that casting between normal references and extern type references like this is OK, but it seems reasonable? |
This comment has been minimized.
This comment has been minimized.
|
See 47e932b for why RustString is in |
Zalathar
force-pushed
the
rust-string
branch
4 times, most recently
from
a1dcb02 to
730d088
Compare
|
☔ The latest upstream changes (presumably #132762) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Rebased to resolve conflicts and remove more ignore/expect added by (my) other PRs. |
| #![warn(unreachable_pub)] | ||
| // tidy-alphabetical-end | ||
|
|
||
| // NOTE: This crate only exists to allow linking on mingw targets. |
There was a problem hiding this comment.
I'm not sure what this was originally referring to, but it's pretty clearly no longer true of the present crate.
cuviper
left a comment
cuviper
left a comment
There was a problem hiding this comment.
Overall it looks like a nice improvement! I just have a couple quibbles.
rustbot
added
S-waiting-on-author
|
Addressed review feedback (diff). |
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
@bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (012ae13): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (secondary 3.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 780.614s -> 780.969s (0.05%) |
Make `RustString` an extern type to avoid `improper_ctypes` warnings Currently, any FFI function that uses `&RustString` needs to also add `#[ignore(improper_ctypes)]` to silence a warning. The warning is not _completely_ bogus, because `RustString` contains `Vec<u8>` and therefore does not have a guaranteed layout. But we have no way of telling the lint that this doesn't matter, because the C++ code only uses that pointer opaquely and never relies on its underlying layout. Ideally there would be some way to silence `improper_ctypes` at the type-definition site. But because there isn't, casting to and from a separate extern type is better than having to annotate every single use site.
6 participants
Currently, any FFI function that uses
&RustStringneeds to also add#[ignore(improper_ctypes)]to silence a warning.The warning is not completely bogus, because
RustStringcontainsVec<u8>and therefore does not have a guaranteed layout. But we have no way of telling the lint that this doesn't matter, because the C++ code only uses that pointer opaquely and never relies on its underlying layout.Ideally there would be some way to silence
improper_ctypesat the type-definition site. But because there isn't, casting to and from a separate extern type is better than having to annotate every single use site.