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

Replace remove() with setting to null #2987

Closed
wants to merge 3 commits into from
Closed

Conversation

ianswett
Copy link
Contributor

Fixes #2569

@ianswett ianswett added -recovery editorial An issue that does not affect the design of the protocol; does not require consensus. labels Aug 25, 2019
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.

On line 1098, where it says:

  if (sent_packets[pn_space][ack.largest_acked] &&
      IncludesAckEliciting(newly_acked_packets)):
    latest_rtt =

Should you instead be really clear and say:

  if (sent_packets[pn_space][ack.largest_acked] != null &&
      IncludesAckEliciting(newly_acked_packets)):
    latest_rtt =

@mikkelfj
Copy link
Contributor

It's a matter of taste, but I don't really like setting values to null when null is not defined value in a set. Removing something from a set is mathematically well defined. That Javascript happens to be weird doesn't have to affect a specification.

ianswett and others added 2 commits August 27, 2019 01:14
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
@ianswett
Copy link
Contributor Author

@mikkelfj I was happy with remove() as well but two issues were filed that both seemed to indicate that using remove() was confusing. I'm open to other resolutions as well(or doing nothing).

@martinthomson
Copy link
Member

I personally would prefer to keep remove(x) and use something like contains(x) on the test, but I trust Ian's judgment.

@mikkelfj
Copy link
Contributor

I suspect the issues are about operational behaviour more than formalism, and a null test is just a proposed solution. If so, I think remove and contains is a much better approach.

ianswett added a commit that referenced this pull request Sep 10, 2019
Per @martinthomson suggestion, use contains() instead of the implicit check.

Fixes #2569 
An alternative to #2987
@ianswett
Copy link
Contributor Author

I created a 1-line PR #3015 to use contains(). I slightly prefer that one to this one.

@janaiyengar
Copy link
Contributor

I like #3015 better.

ianswett added a commit that referenced this pull request Sep 10, 2019
Per @martinthomson suggestion, use contains() instead of the implicit check.

Fixes #2569 
An alternative to #2987
@ianswett ianswett closed this Sep 10, 2019
@MikeBishop MikeBishop deleted the ianswett-set-to-null branch March 18, 2020 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RTT sample should be taken only once for each largest_acked
4 participants