Integrate rustfmt into the build system and/or Continuous Integration

[briansmith]

[Edit: This is just about rustfmt now; Clippy integration is now #448]

#277 shows that Clippy is useful. We should integrate it into the build system and/or the continuous integration so that we can catch problems before they get committed.

On Continue Integration, Clippy checks should run AFTER the tests. I think it would be too annoying to have a push fail to even run the tests because of a formatting error.

It's probably best to use the cargo clippy subcommand instead of the compiler plugin, as I suspect it would be less brittle and more likely to work across stable/beta/nightly Rust.

[briansmith]

See also http://johannh.me/blog/rustfmt-ci.html

[pietro]

rustfmt currently does a lot of changes and breaks some code that has inline C-style block comments.

I guess the goal would be to take some of the changes, ignore some and add attributes to make rustfmt ignore some code blocks.

Can you give some examples of things that should not to be changed? This is the diff from running rustfmt on current master:
https://github.com/pietro/ring/tree/rust_fmt

[briansmith]

The things I most strongly object to are mentioned in my review comments in #385 (review). In general I very much like the idea of rustfmt, but I would like to see rustfmt fixed and/or made more flexible to output formatting that is optimized for minimizing the number of lines of code and optimized for the 80-column width, due mostly to limitations of my code reviewing system. Unless/until that happens I think it makes less sense to try to reformat more than what's already been done, because people won't generally be able to do rustfmt to automatically format their code until rustfmt is fixed.

One thing I'd also like to consider is reformatting everything to use 2 space indent, to compensate for the 80-column width rule. I've always wanted to use 2-space indent but when I started Rust programming I had the impression that nobody did that. Now I know I'm not the only one so i think it makes sense to go ahead with it.

Regarding C-style comments like in_out, we can probably just rename the parameters to have an in_out_ prefix and get rid of the comment.

[pietro]

One thing I'd also like to consider is reformatting everything to use 2 space indent, to compensate for the 80-column width rule. I've always wanted to use 2-space indent but when I started Rust programming I had the impression that nobody did that. Now I know I'm not the only one so i think it makes sense to go ahead with it.

I'm having evil thoughts of running sed on the whole source and doing a gigantic PR which changes to every line :P Anyway, I tried doing it on src/digest/digest.rs let me know what you think and how we can coordinate that. https://gist.github.com/pietro/4294f9894b61a27417a2298eae185198/revisions

Regarding C-style comments like in_out, we can probably just rename the parameters to have an in_out_ prefix and get rid of the comment.

I'll do a small PR to change that. rustfmt can't handle the C-style comments it ends up removing some of the code and leaving syntactically incorrect garbage in its place. I thought about filling a bug report but I think ring is the only rust code base using inline /* */ blocks.

[pietro]

BTW, currently clippy warns about lack of backticks on documentation and the use of too many one char variable names on sha1.rs. I'll do a PR to have it stop complaining about sha1.rs and I'll take a closer look to the documentation issues. Some of the things clippy warns about are definitively wrong, like SpecialPublications in http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf

[briansmith]

I'm going to make this issue about just rustfmt, since that's the most-discussed thing in this. I filed #448 to track automatically verifying we pass Clippy checks, which is something I think we can do sooner and separately from rustfmt.

[briansmith]

BTW, currently clippy warns about lack of backticks on documentation and the use of too many one char variable names on sha1.rs. I'll do a PR to have it stop complaining about sha1.rs

Thanks!

Some of the things clippy warns about are definitively wrong, like SpecialPublications in http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-56Ar2.pdf

I filed https://github.com/Manishearth/rust-clippy/issues/1508.

Regarding the reformatting to switch to two-space indent, I think that should be a separate issue. And I'd like to get through my PR backlog before tackling it.

[pietro]

Regarding C-style comments like in_out, we can probably just rename the parameters to have an in_out_ prefix and get rid of the comment.

I did a quick look on C-style comments and they were all of the form x: *const T/*[N]*/ and x: *mut T/*[N]*/. There's already an issue for changing these. I'll stop playing with rustfmt for now.