Reason for boxing inner values of Value

[baloo]

sea_query::Value makes a fairly unusual choice to box every subvalue.

sea-query/src/value.rs

Line 143 in 90283ad

Bytes(Option<Box<Vec<u8>>>),

The comments seems to indicate that it is to ensure the type can be contained within the size of a single pointer (

sea-query/src/value.rs

Line 121 in 90283ad

/// We want the inner Value to be exactly 1 pointer sized, so anything larger should be boxed.

)

But that seems to no longer be the case?

    #[test]
    fn test_size_value() {
        use std::mem;
        assert_eq!(mem::size_of::<Value>(), 24);
    }

It just might be that code drifted over time, and comment no longer applies,

Asking as a consumer of the crate as this seems to be a fairly convoluted type to use, for no actual benefit?
I would actually expect Value::Bytes to be an Option<Box<[u8]>> and in practice it doesn't look like it would inflate the size of value.

Asking to see if maybe that would be a choice that could be reconsidered? (Before sending a PR)

[tyt2y3]

I agree with your general idea, and I am open to removing the boxes for some types, if you can help decide ultimately do we restrict size of outer Value to be 16 or 24 bytes.

assert_eq!(mem::size_of::<Value>(), 24); can you find out the offender? because it depends on what feature flags are being enabled.

I would rather use the vanilla type though, because Box<str> is not entirely interchangable with String for example.

[baloo]

I haven't chased the offender yet. I think I remember that was the default build.
I guess I'll just add a test to check the sizes (will need to be parametric on the platform pointer sizes most likely).

I agree about Box<str> not being equivalent to String, but I view String as being essentially Box<str> where I have issues is the Box<String> or the Box<Vec> (double heap allocation with minimal benefit).

Anyway, thanks for the input! I'll see what I can do :)