Subject: [quicwg/base-drafts] Replace remove() with setting to null (#2987)

From: ianswett <[email-masked]>
Date: Fri, 23 Aug 2019 08:34:35 -0700

Fixes #2569
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/2987

-- Commit Summary --

  * Replace remove() with setting to null

-- File Changes --

    M draft-ietf-quic-recovery.md (4)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/2987.patch
https://github.com/quicwg/base-drafts/pull/2987.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987

---

From: Martin Thomson <[email-masked]>
Date: Sun, 25 Aug 2019 18:07:51 -0700

martinthomson commented on this pull request.

> @@ -1157,7 +1157,7 @@ Pseudocode for OnPacketAcked follows:
    OnPacketAcked(acked_packet, pn_space):
      if (acked_packet.in_flight):
        OnPacketAckedCC(acked_packet)
-     sent_packets[pn_space].remove(acked_packet.packet_number)
+     sent_packets[pn_space].[acked_packet.packet_number] = null

```suggestion
     sent_packets[pn_space][acked_packet.packet_number] = null
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987#pullrequestreview-279365021

---

From: Martin Thomson <[email-masked]>
Date: Sun, 25 Aug 2019 18:10:37 -0700

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 =
```

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987#pullrequestreview-279365304

---

From: MikkelFJ <[email-masked]>
Date: Sun, 25 Aug 2019 23:56:46 -0700

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987#issuecomment-524741093

---

From: ianswett <[email-masked]>
Date: Mon, 26 Aug 2019 22:14:38 -0700

@ianswett pushed 1 commit.

357c25fdaea957c37822f3a57478c9b6d5649aa9  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987/files/6a0268e0806ac74eefe3d0897e4c712f0dffc519..357c25fdaea957c37822f3a57478c9b6d5649aa9

---

From: ianswett <[email-masked]>
Date: Mon, 26 Aug 2019 22:15:50 -0700

9b9faa8935345c937811a4ccdc73d705ed5fb3cc  Martin's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987/files/357c25fdaea957c37822f3a57478c9b6d5649aa9..9b9faa8935345c937811a4ccdc73d705ed5fb3cc

---

From: ianswett <[email-masked]>
Date: Mon, 26 Aug 2019 22:17:20 -0700

@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).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987#issuecomment-525141265

---

From: Martin Thomson <[email-masked]>
Date: Mon, 26 Aug 2019 22:28:28 -0700

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

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987#issuecomment-525143864

---

From: MikkelFJ <[email-masked]>
Date: Tue, 27 Aug 2019 00:01:03 -0700

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2987#issuecomment-525168184