-
Notifications
You must be signed in to change notification settings - Fork 703
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
Apply rustfmt. #385
Apply rustfmt. #385
Conversation
Various broken cases handled manually. Also, add a Travis hook and a sample pre-commit hook.
Aesthetically I don't necessarily like some of the edits rustfmt applies, but if rustfmt checks via CI are the goal (and I do like consistent formatting), then I don't see a way around doing what it wants. |
I agree to license my contributions to each file under the terms given at the top of each file I changed.
I agree to license my contributions to each file under the terms given at the top of each file I changed.
@@ -18,16 +18,20 @@ use ring::*; | |||
use std::error::Error; | |||
use std::io::{Read, Write}; | |||
|
|||
|
|||
#[cfg_attr(rustfmt, rustfmt_skip)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustfmt handles multi line string literals poorly at the moment
@@ -26,7 +26,8 @@ | |||
//! `padding_length||payload||random padding`. | |||
//! | |||
//! [[email protected]]: | |||
//! http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.chacha20poly1305?annotate=HEAD | |||
//! http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This url-line-breaking is pretty unfortunate but I didn't see any other way to appease rustfmt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fix rustfmt instead. I think that keeping URLs intact should be an uncontroversial change to rustfmt, at least as an option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already a bug: rust-lang/rustfmt#506
pub fn positive_integer<'a>(input: &mut untrusted::Reader<'a>) | ||
-> Result<untrusted::Input<'a>, error::Unspecified> { | ||
pub fn positive_integer<'a> | ||
(input: &mut untrusted::Reader<'a>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting is also very strange
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems like maybe a rustfmt bug? When it does this to a where
clause it puts the opening brace on a new line. Not sure why it doesn't do that in this case.
} | ||
// We don't increase |self.completed_data_blocks| because the | ||
// padding isn't data, and so it isn't included in the data length. | ||
padding_pos = 0; | ||
} | ||
|
||
polyfill::slice::fill( | ||
&mut self.pending[padding_pos..(self.algorithm.block_len - 8)], 0); | ||
let slice = self.pending[padding_pos..(self.algorithm.block_len - 8)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the inline calculation in the slice confused rustfmt mightily so I pulled out a local
a: *const Limb/*[COMMON_OPS.num_limbs]*/); | ||
fn GFp_p256_scalar_sqr_rep_mont(r: *mut Limb/*[COMMON_OPS.num_limbs]*/, | ||
a: *const Limb/*[COMMON_OPS.num_limbs]*/, | ||
fn GFp_nistz256_add(// [COMMON_OPS.num_limbs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to make these //
comments so that it wouldn't try to put them on the same line, and then fall afoul of the line length limit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to just tell rustfmt to ignore this block.
@@ -210,6 +211,7 @@ fn derive_block(secret: &hmac::SigningKey, iterations: usize, salt: &[u8], | |||
/// | |||
/// `derive` panics if `out.len()` is larger than (2**32 - 1) * the PRF digest | |||
/// length, per the PBKDF2 specification. | |||
#[cfg_attr(rustfmt, rustfmt_skip)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function also made rustfmt mad.
let aligned_buf = slice_as_array_ref_mut!(&mut buf[offset..(offset + | ||
OPAQUE_LEN)], | ||
OPAQUE_LEN) | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yikes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already tried to do this. I would very much like to do so, and I don't think I have particularly strong preferences for code formatting in general, but IMO rustfmt is doing unreasonable things to the code.
I suggest we just punt on this for now, though if you want to go through and resubmit the no-brainer changes, I would review that.
tag, | ||
nonce, | ||
ad.as_ptr(), | ||
ad.len()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is most of the reason why we don't use rustfmt already. We need a formatting tool that it can format function calls in the compact format, and that generally optimizes for readability first and minimum number of lines second, like the Google clang-format style.
Alternatively, we might be able to use the default rustfmt style after we've oxidized more of ring so that there aren't so many function calls that take a large number of arguments. But not yet.
}) | ||
} | ||
|
||
fn aes_gcm_open(ctx: &[u64; aead::KEY_CTX_BUF_ELEMS], | ||
nonce: &[u8; aead::NONCE_LEN], in_out: &mut [u8], | ||
in_prefix_len: usize, tag_out: &mut [u8; aead::TAG_LEN], | ||
ad: &[u8]) -> Result<(), error::Unspecified> { | ||
ad: &[u8]) | ||
-> Result<(), error::Unspecified> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not such a big deal as the function call formatting, but still it seems unnecessary.
@@ -26,7 +26,8 @@ | |||
//! `padding_length||payload||random padding`. | |||
//! | |||
//! [[email protected]]: | |||
//! http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.chacha20poly1305?annotate=HEAD | |||
//! http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fix rustfmt instead. I think that keeping URLs intact should be an uncontroversial change to rustfmt, at least as an option.
pub fn positive_integer<'a>(input: &mut untrusted::Reader<'a>) | ||
-> Result<untrusted::Input<'a>, error::Unspecified> { | ||
pub fn positive_integer<'a> | ||
(input: &mut untrusted::Reader<'a>) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems like maybe a rustfmt bug? When it does this to a where
clause it puts the opening brace on a new line. Not sure why it doesn't do that in this case.
@@ -221,7 +223,8 @@ mod tests { | |||
&[0x02, 0x00, 0x01], | |||
&[0x02, 0x01], | |||
&[0x02, 0x01, 0x00, 0x01], | |||
&[0x02, 0x01, 0x01, 0x00], // Would be valid if last byte is ignored. | |||
/* Would be valid if last byte is ignored. */ | |||
&[0x02, 0x01, 0x01, 0x00], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably this could be written with a C++ comment in that position instead of a C-style comment?
a: *const Limb/*[COMMON_OPS.num_limbs]*/); | ||
fn GFp_p256_scalar_sqr_rep_mont(r: *mut Limb/*[COMMON_OPS.num_limbs]*/, | ||
a: *const Limb/*[COMMON_OPS.num_limbs]*/, | ||
fn GFp_nistz256_add(// [COMMON_OPS.num_limbs] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to just tell rustfmt to ignore this block.
I'm fine with punting. I'll close this (and submit some rustfmt bugs). I just picked it up because it looked like something I could get done while you looked at my other PR's. ;) It's not like this project had egregiously sloppy code formatting to begin with, so the win isn't very significant when it comes with so many annoying mis-formats. |
Various broken cases handled manually.
Also, add a Travis hook and a sample pre-commit hook.
(Addresses #285)