search

getlantern / natty

Standalone WebRTC-based NAT traversal
Apache License 2.0
97 stars 21 forks source link

This pull request just exists to have a place to put comments. Please don't pull! #6

Closed oxtoacart closed 3 years ago

oxtoacart commented 9 years ago

@atavism I've finished reviewing. Looks good overall. There's one serious comment on Natty::InputStream::read and then just a bunch of niggles/suggestions.

oxtoacart commented 9 years ago

Oh, I should qualify this by saying that my C++ is mediocre at best, so I was looking for logic/structural type things. I will have missed anything involving finer points of C++ specific practices/idioms.

atavism commented 9 years ago

@oxtoacart, I made all of the changes you recommended, but I committed them to master already. Should I create a PR on master and an earlier commit, or would you rather just glance over the set of changes here? https://github.com/getlantern/natty/compare/8546621ce073508a29019785b6e1200a378f9808...master#diff-69ff2f504b5995b5aefdd25745df7a45R179

atavism commented 9 years ago

Oh, one minor detail: since we're making the STUN servers configurable from the command line, does it make sense for natty to immediately exit if it's passed an invalid one? If so, I'll go ahead and make that change.

oxtoacart commented 9 years ago

Yes, I think that it makes sense for natty to exit if it is given any invalid stun servers.

Cheers, Ox

"I love people who harness themselves, an ox to a heavy cart, who pull like water buffalo, with massive patience, who strain in the mud and the muck to move things forward, who do what has to be done, again and again."

On Wed, Dec 3, 2014 at 11:25 PM, atavism notifications@github.com wrote:

Oh, one minor detail: since we're making the STUN servers configurable from the command line, does it make sense for natty to immediately exit if it's passed an invalid one? If so, I'll go ahead and make that change.

— Reply to this email directly or view it on GitHub https://github.com/getlantern/natty/pull/6#issuecomment-65539577.