nical
commented
4 years ago As a continuation of our discourse about by value vs by reference: added translation methods for Boxes takes its by reference, but as godbolt shows, even without forced #[inline] directive compiler smart enough to inline such small function and the code the same in both cases.
I salute your approach of checking things in godbolt beforehand. It's the right thing to do, but there are a few traps to be aware of when drawing conclusions:
- In general, inlining is a tricky optimization to simulate: the smaller/simpler your test programs/functions are, the easier it is for the compiler to inline things. It can depend on the function to inline but also where the function is potentially inlined.
- Inlining is more likely to happen within a crate than when calling a public function or method defined in another crate. Rustc is particularly shy about inlining across crates and it's hard to observe this kind of effect in tools like godbolt.
Beyond that, there are simple and complex functions manipulating rects and boxes. I prefer to be consistent about how each type is passed (value or ref) basing on a simple rule (size of the struct) without penalizing functions that cant or shouldn't be inlined, than look at each function and try to see if a particular version of the compiler inlines them (with the risks of extrapolating from simplified test cases).
You'd be right to point out that there are some inconsistencies in euclid today (for example 2d points are passed by value in most places but there are a functions that pass them by ref), and these are good candidates for fixing in the next wave of breaking change.
Note that the size of the struct is the heuristic that static analyzers like clippy and clang tools in C++ land use to advise for or against passing copyable structures by value. I just had a look at clippy's source code and it uses 8 bytes as the threshold above which they don't nudge you towards passing structures by value.
Anyway, the patch looks good, thanks. @bors-servo r+
bors-servo
As a continuation of our discourse about by value vs by reference: added translation methods for Boxes takes its by reference, but as godbolt shows, even without forced
#[inline]directive compiler smart enough to inline such small function and the code the same in both cases. Therefore, I'm still in favour of taking everything by value in methods that almost only change unit. For now I'm, however, use reference as in surrounding code.