bit2swaz commented on PR #10431: URL: https://github.com/apache/arrow-rs/pull/10431#issuecomment-5094882022
yeah youre both right, ill make all the fields private. i was wrong that the data fields were harmless, my bad on that @ashdnazg's point on `format`/`name` is valid (setting them to anything that didnt come from `CString::into_raw` blows up in `release`) and @alamb's `buffers` example is the same thing on the array side. `buffer()` does a `read_unaligned` on `self.buffers`, so a bad pointer there is UB with no `unsafe` at the call site. so any write to the inner fields has to be treated as unsafe. going all-fields-private reads are already covered, all three structs have typed getters for every field, so nothing new needed there on the write side, ill keep the per-field `unsafe` setters (`set_release` / `set_private_data`). those are the only fields the #9771 wrap-release flow actually writes, and like you said @alamb, setters don't force an api change when a field gets added later. the `unsafe fn new_from_parts(...)` idea is nice for building a struct from scratch but the safe `try_from` / `new` constructors already cover that path and nobodys asked to hand-build these field by field yet, so id rather not add it speculatively. easy to tack on later if a use case shows up. lmk if youd rather have it now tho the `with_metadata` thing ill spin off into its own issue later like i mentioned -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
