Re: [quicwg/base-drafts] [qpack] Overhaul the pseudocode (#3577)

[Martin Thomson <[e-mail]>]

@martinthomson commented on this pull request.

I couldn't see any problems, but I have some suggestions.

Thanks for doing the cleanup.

>  for header in headers:
-  staticIdx = staticTable.getIndex(header)
-  if staticIdx:
-    encodeIndexReference(streamBuffer, staticIdx)
+  staticIndex = staticTable.getIndex(header)

```suggestion
  staticIndex = staticTable.findIndex(header)
```

This is a search, right?

>      continue
 
-  dynamicIdx = dynamicTable.getIndex(header)
-  if !dynamicIdx:
+  dynamicIndex = dynamicTable.getIndex(header)

```suggestion
  dynamicIndex = dynamicTable.findIndex(header)
```

>      # No matching entry.  Either insert+index or encode literal
-    nameIdx = getNameIndex(header)
+
+    # getNameIndex attempts to find an index for this header's name.
+    # If one exists, it returns the name's (absolute) index and a
+    # boolean indicating which table has the name.  If the name
+    # can't be found, nameIndex is None
+    nameIndex, isStaticName = getNameIndex(header.name)

This is a little strange, as you are relying on functions on the tables to find the full entry, but you jump to a different approach for looking up the name.  I realize that two lookups are worse than one, but it's an odd stylistic jump, that's all.

```python
# An alternative might be to do the name lookup like this:
staticNameIndex = staticTable.findName(header.name)
if staticNameIndex is None:
    dynamicNameIndex = dynamicTable.findName(header.name)
if header.canIndex() and dynamicTable.canAdd(header):
    encodeInsert(encoderBuffer, staticNameIndex, dynamicNameIndex, header)
    dynamicIndex = dynamicTable.add(header)

    # For a bonus, continuing from the above, forget about the
    # newly inserted dynamic table entry if we are at the limit of blocked streams.
    if numBlockedStreams >= maxBlockedStreams:
        dynamicIndex = None
    else:
        addedBlockingEntry = True
        # Later, you can increase numBlockedStreams, but you only do that once.
```

>        dynamicTable.add(header)
-      dynamicIdx = dynamicTable.baseIndex
+      dynamicIndex = dynamicTable.getInsertCount()

I can't suggest here, but I think that you just want this:

```python
        dynamicIndex = dynamicTable.add(header)
```

Otherwise you are breaking encapsulation in a small way: relying on the insert count for getting the index of the thing you just added.

-- 
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/3577#pullrequestreview-392540915