refactor: cleanup and changes to satisfy Clippy

[rjzak]

I've been working on addressing Clippy warnings and this ended up being bigger than I expected and there are still a lot of unaddressed warnings/errors.

So I've got some questions:

• Why did the serialization/deserialization functions take a boolean value which seems to not be used?
• Can commented out code be removed? Some commented out items were for debugging output, these could instead be preserved with the tracing crate and info!() or trace!() macros.
• Is there a style guide or are Clippy & cargo fmt defaults sufficient?
• Would the project be okay with more error handling to remove some of the .unwrap()s?
• Am I on the right track for a useful contribution?

Tests should still pass, the ones which fail (25 of them at the end) are because the script seems to not find the release compiled examples, despite me compiling them and the other tests being able to use them.

[rdfriese]

Thanks for the effort here, its definitely appreciated and needed as we transition this from more research oriented code to stable code.

• Why did the serialization/deserialization functions take a boolean value which seems to not be used? -- At one point we were planning to explore variable length serialization and added the API, but we weren't convinced the effort would be worth it, I think its fine to remove that bool from all those calls.
• Can commented out code be removed? -- good question I'd probably say generally yes
• Some commented out items were for debugging output, these could instead be preserved with the tracing crate and info!() or trace!() macros. -- agreed, we've been trying to be a bit better about consistently using these
• Is there a style guide or are Clippy & cargo fmt defaults sufficient? -- we have generally stuck to the defaults, but are happy to talk about any suggestions!
• Would the project be okay with more error handling to remove some of the .unwrap()s? -- I think that would be great!
• Am I on the right track for a useful contribution? I think so, I certainly appreciate it!

[rjzak]

There are a few areas with a mutable reference is returned from a function which doesn't get a mutable reference to self. What are your thoughts on this?

note: immutable borrow here
   --> src/memregion.rs:127:32
    |
127 |     pub unsafe fn as_mut_slice(&self) -> MemResult<&mut [T]> {
    |                                ^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#mut_from_ref

Maybe this isn't a big deal since it's already unsafe. Maybe the reference should be mutable to the user knows then calling the function that the object may be changed?

So this can be made mutable, or we can whitelist such Clippy concerns.

[rdfriese]

Hmm lets try having it accept a mutable reference to self and see if that causes any major issues, given that its a public interface we should probably try to adopt standard rust design patterns

[rjzak]

What are your thoughts on this PR? I know it's pretty big, but it does make decent progress towards Clippy:

• This PR now: 370 previous errors; 275 warnings emitted
• master 0141608: 409 previous errors; 571 warnings emitted

Some fixes are to whitelist some Clippy warnings/errors because the changes would drastically change the API, such as functions returning mutable references have to take a mutable reference to the object.

Additionally, this does remove the extra bool for serialization but doesn't address the dual serialization (serialize to get the size, allocate, serialize again into the buffer). Clippy has been useful and reduced some unneeded casts and clones.

[rdfriese]

This looks all good to me! One thing I've ran into in testing is for more complicated tests debug is too slow, so I test in release mode often. Do you know if we can create a new profile like profile.rel_debug or something that enables the debug_asserts while also keeping the release performance optimizations?

[rjzak]

It seems you can by specifying debug-assertions = true in your new compilation profile.

https://doc.rust-lang.org/beta/cargo/reference/profiles.html#debug-assertions

I changed some of those assert statements to try to make the release mode faster, but wasn't aware of the slow debug issue. I like the "release debug" profile idea.