Subject: [quicwg/base-drafts] Add a check for excessive line length in figures (#304)

From: Martin Thomson <[email-masked]>
Date: Tue, 14 Feb 2017 16:18:51 -0800

@ianswett, this is for your benefit largely.  It can't fix the problems it finds, but it should help you identify them early.  Can you double-check that my refactoring of your algorithm is correct?
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Add a check for excessive line length in figures

-- File Changes --

    M Makefile (13)
    M draft-ietf-quic-recovery.md (7)
    M draft-ietf-quic-transport.md (4)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/304.patch
https://github.com/quicwg/base-drafts/pull/304.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/304

---

From: ianswett <[email-masked]>
Date: Wed, 15 Feb 2017 06:10:51 -0800

ianswett commented on this pull request.

Time and packet number spaces are different, sadly.

Thanks for the improved Makefile, though.  Very helpful.

>           lost_packets.insert(unacked_packet.packet_number);
-       else if (unacked_packet.packet_number <
-                acked_packet.packet_number - reordering_threshold)
+       else if (delta > reordering_threshold)

The first check is a time delta and the second check is a packet number delta.  If you want to define a time_delta and packet_delta for readability, that would work.

Rearranging the conditionals does make it easier to read, so thanks for that.

-- 
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/304#pullrequestreview-22006500

---

From: Martin Thomson <[email-masked]>
Date: Wed, 15 Feb 2017 17:18:30 -0800

martinthomson commented on this pull request.

Ugh, I realized this yesterday, fixed it, but failed to push the update.  On it.

-- 
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/304#discussion_r101425844

---

From: Martin Thomson <[email-masked]>
Date: Wed, 15 Feb 2017 17:19:54 -0800

@martinthomson pushed 2 commits.

31b92d2  Making more extensive changes
51d962b  Using Ian's suggested names

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/304/files/7b962e8d7e847ab15583cda18495e0b2d09d1c4c..51d962b999179193ec0530dd56c8fe61948d4c2d

---

From: ianswett <[email-masked]>
Date: Thu, 16 Feb 2017 07:43:53 -0800

ianswett approved this pull request.

One nit on consistency, but looks good.

> @@ -406,7 +406,7 @@ Pseudocode for OnAlarm follows:
 ~~~
    OnAlarm(acked_packet):
      lost_packets = DetectLostPackets(acked_packet);

Below you changed DetectLostPackets to accept acked, but here it's still acked_packet.

-- 
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/304#pullrequestreview-22280802

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Feb 2017 16:35:16 -0800

~~~
    OnAlarm(acked_packet):

Yes, I figured that it would be OK to change the name of a "local" variable in that context.  I was going to change this, but then I have to change a bunch of other things and it started to cascade.  I'll let you sort that mess out :)

-- 
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/304#discussion_r101662474

---

From: Martin Thomson <[email-masked]>
Date: Thu, 16 Feb 2017 16:35:20 -0800

Merged #304.

-- 
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/304#event-966032375