Skip to content
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

Toggle pedantic on neqo common #374

Merged
merged 3 commits into from
Jan 7, 2020

Conversation

o0Ignition0o
Copy link
Collaborator

@o0Ignition0o o0Ignition0o commented Dec 29, 2019

  • Fixed new clippy (default) lints
  • Fixed new pedantic lints in neqo-crypto
  • Toggled pedantic in neqo-common

Please let me know if you would like me to squash the commits, and if there's anything I can do to improve the PR :)

// The directive is placed here because
// we cannot stack directives.
// Placing it on top of #[must_use] doesn't work.
#[allow(clippy::unused_self)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this lint did not exist for the toolchain our CI uses.
Not really sure if/when we want to bump versions, so I'll remove this directive for now.
Let's keep in mind we will have to add it again once we update our clippy/rustc versions in the CI

let (fin, fill) = if data.len() > remaining {
if remaining == 0 {
return None;
let (fin, fill) = match data.len().cmp(&remaining) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to conflict with #370.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's wait until #370 lands then so I can rebase :)

@@ -523,7 +523,7 @@ mod tests {
let flow_mgr = Rc::new(RefCell::new(FlowMgr::default()));
let conn_events = ConnectionEvents::default();

let mut s = RecvStream::new(567.into(), 1024, flow_mgr.clone(), conn_events.clone());
let mut s = RecvStream::new(567.into(), 1024, flow_mgr, conn_events);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that I follow this change. These are Rc<> instances that presumably we want to clone for passing into multiple invocations of this function. Has Rc picked up an implementation of Copy in a recent version of rust?

I would have thought that moving to Rc::clone(&flow_mgr) would be wise, but I don't see that here either.

Copy link
Collaborator Author

@o0Ignition0o o0Ignition0o Jan 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice catch, I'll move it to Rc::clone() to make it more explicit.
I think I got fooled by the linter, which advises to remove .clone(), which doesn't make sense in an Rc context.

It might be worth opening an issue to clippy.

Edit: just did rust-lang/rust-clippy#4982

neqo-transport/src/connection.rs Show resolved Hide resolved
@o0Ignition0o
Copy link
Collaborator Author

Rebased against master, now that #370 has merged

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two Rc::clone() instances and this is great. Thanks for doing all of this!

neqo-transport/src/send_stream.rs Outdated Show resolved Hide resolved
neqo-transport/src/send_stream.rs Show resolved Hide resolved
@mergify mergify bot merged commit c6bc802 into mozilla:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants