New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide `NativeEndian` default for generic `Endianity` type parameters #163

Closed
fitzgen opened this Issue Dec 7, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@fitzgen
Copy link
Member

fitzgen commented Dec 7, 2016

Almost all of our code is generic over an Endian type parameter, with the bound where Endian: Endianity.

We should default to the NativeEndianity for these type parameters.

So this

struct Whatever<Endian>
    where Endian: Endianity
{
    ...
}

would become

struct Whatever<Endian = NativeEndianity>
    where Endian: Endianity
{
    ...
}

We could do this all in one go, or incrementally with a PR for a each module at a time. Whatever works for whoever wants to pick this up.

@Geemili

This comment has been minimized.

Copy link

Geemili commented Dec 14, 2016

Can I take this?

@fitzgen

This comment has been minimized.

Copy link
Member

fitzgen commented Dec 14, 2016

@Geemili sure thing! Let me know if you have any questions :)

@fitzgen fitzgen added the assigned label Dec 14, 2016

@Geemili

This comment has been minimized.

Copy link

Geemili commented Dec 30, 2016

@fitzgen I'm sorry, but I haven't been able to make time for this. If someone else wants to take this issue, they can.

@fitzgen

This comment has been minimized.

Copy link
Member

fitzgen commented Dec 30, 2016

No problem :)

@fitzgen fitzgen removed the assigned label Dec 30, 2016

@gchamp20

This comment has been minimized.

Copy link

gchamp20 commented Aug 4, 2017

Hi,

I was looking into this and I think this is not an issue anymore with the introduction of the Reader trait.
Most structures that would have benefited from this now look like:

struct Whatever<R>
    where R: Reader  
{
    ...  
}  

The endianity is passed as a parameter to the builder pattern. We can't use optional parameters (yet.. RFC #323), so I assume there's no way of implementing this currently?

@philipc

This comment has been minimized.

Copy link
Collaborator

philipc commented Aug 4, 2017

Yeah I think you're right. The only place I can see that could still default is on struct EndianBuf, but that doesn't help because it has an endian field now, so would need optional parameters too. Plus I'm not sure that using an optional endian parameter is a good idea when using RunTimeEndian. That might lead to obscure bugs.

We also can't default Reader = EndianBuf<'input, NativeEndian> because of the lifetime parameter.

Thanks for looking into this!

@philipc philipc closed this Aug 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment