Skip to content

Adds ARP protocol.#5

Open
lehouatais wants to merge 2 commits intomeh:masterfrom
lehouatais:master
Open

Adds ARP protocol.#5
lehouatais wants to merge 2 commits intomeh:masterfrom
lehouatais:master

Conversation

@lehouatais
Copy link
Copy Markdown

Contributes to issue #3

Copy link
Copy Markdown
Owner

@meh meh left a comment

Choose a reason for hiding this comment

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

I hate this review interface that doesn't make it obvious there are pending notes on the thing, and then you don't get any more notification and you forget you were alive and this happens, 5 years later you get minor nitpicks.

Sorry about that, if you're not interested in doing these changes anymore I'll address the nitpicks myself, trying to catch up with the backlog of pull requests and issues on all my projects.

Comment thread src/ether/builder.rs
if self.payload {
return Err(ErrorKind::AlreadyDefined.into());
}
self = self.protocol(Protocol::Arp)?;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you use tabs here please?

Comment thread src/arp/packet.rs

impl<B: AsRef<[u8]>> Packet<B> {

pub fn hardware_type(&self) -> u16 {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you use tabs here?

Comment thread src/arp/builder.rs
payload: bool,
}


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could you remove this empty line?

Comment thread src/arp/builder.rs
Packet::new(self.buffer.data_mut())
}
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This one too.

Comment thread src/arp/builder.rs
Packet::unchecked(self.buffer.data_mut()).set_target_protocol_address(value)?;
Ok(self)
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

And this one.

Comment thread src/arp/packet.rs
}

impl<B: AsRef<[u8]>> Packet<B> {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

And this one.

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