Alloc methods should take layout by reference

[joshlf]

Currently, the methods of the Alloc trait that take a Layout take it by value. This is unnecessary, and makes clippy unhappy. Instead, they should take their Layout arguments by reference.

[nagisa]

I feel that Layout being passed by-value is very intended. It is 2-usize worth of data, which will usually be split over registers for the call and end up being more efficient than going through a reference.

[nagisa]

clippy warning about this seems like a false positive though.

[joshlf]

Fair enough. @Manishearth do you have thoughts on clippy's behavior here? Is this just one of those situations in which the right move is to silence clippy with an allow(needless_pass_by_value)?

[Manishearth]

Hmm, Layout is Copy, Clippy shouldn't be warning on Copy period. Will have a look

[joshlf]

Doesn't look like it's Copy: https://doc.rust-lang.org/src/alloc/allocator.rs.html#46

[Manishearth]

That seems incorrect?

In this case the clippy lint is working as it should; taking non-copy types by-move isn't really good. If you're doing this for perf reasons slap on an allow.

[joshlf]

Incorrect in that Layout ought to be Copy or incorrect in that I shouldn't be complaining about it if clippy is in the right? :)

[Manishearth]

Layout should be Copy.

If it's not because it's considered a hazard (much like how Range isn't Copy) then it should be passed by reference because otherwise it gets consumed each time and needs explicit cloning.

[joshlf]

Also, IIRC, the reason that it's not Copy is that somebody (I believe it was @SimonSapin ?) wanted to leave the door open to having Layouts be able to contain a chain of other Layouts from which they were derived, which would preclude them from being Copy. As I see it:

• If we go that route, then Layouts are expensive to clone, and so should be passed by reference
• If we don't go that route, then Layouts should be Copy

Layout should be Copy.

Given the rest of this comment, I tend to agree.

[Manishearth]

Yeah, that reasoning makes sense. In that case these APIs definitely should operate by-ref.

[Manishearth]

cc @gankro

[SimonSapin]

@joshlf This is the first I hear of Layouts being (possibly) chained.

(FWIW I care less about the details of the Alloc trait than I care about stabilizing it within months rather than years.)

[vitalyd]

Hmm, I don't see a compelling reason to make Layout Copy right now and carry that contract. The comment earlier about it being 2 words and thus splittable across registers can also looked at as:

• it might be a downside on register starved arch
• it might not matter if callee is inlined and compiler can shuffle the code as it sees fit

All that to say, I don't think how the optimizer is going to pass it around should be the criteria to decide on Copy (or move) or not.