rust-mobile / ndk

Rust bindings to the Android NDK
Apache License 2.0
1.11k stars 110 forks source link

ndk/media_format: Make all mutating functions take `self` by `&mut` #452

Closed MarijnS95 closed 7 months ago

MarijnS95 commented 9 months ago

Fixes #451, CC @zarik5

All setters can inherently change the internals of the format, and should furthermore invalidate - or not be allowed while - borrowed strings or buffers (are) acquired from it.

Unforatunately getString() is documented as:

The returned string is owned by the format, and remains valid until the next call to getString, or until the format is deleted.

Meaning that it is not reentrant, and invalidates any data previously acquired from str(); hence this getter is marked as &mut self too. The same is however not the case for getBuffer().

spencercw commented 9 months ago

The doc comments on buffer and str seem a little redundant because the rules are enforced by the type system anyway. The str comment is also not strictly accurate because the string is only invalidated when you re-fetch the same key.^1 Not suggesting we should add document this implementation detail, but I don't think the adapted NDK docs are adding much value.

Looks good otherwise.

MarijnS95 commented 9 months ago

Fair enough for buffer, I mostly copied the NDK docs for the few (two) functions that actually have docs upstream.

I'd like to keep "something" for fn str() however, to justify to documentation readers why a getter is borrowing self mutably.

spencercw commented 9 months ago

Yes that makes sense. A mut getter does look a bit odd on the face of it.