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

QPACK Appendix B: nameIndex #3544

Closed
kazu-yamamoto opened this issue Mar 24, 2020 · 5 comments · Fixed by #3577
Closed

QPACK Appendix B: nameIndex #3544

kazu-yamamoto opened this issue Mar 24, 2020 · 5 comments · Fixed by #3577
Labels
-qpack editorial An issue that does not affect the design of the protocol; does not require consensus.

Comments

@kazu-yamamoto
Copy link
Contributor

It seems to me that nameIndex appears suddenly in Appendix B. What's that? Is it the same as nameIdx?

@kazu-yamamoto kazu-yamamoto changed the title Appendix B: nameIndex QPACK Appendix B: nameIndex Mar 24, 2020
@martinthomson martinthomson added -qpack editorial An issue that does not affect the design of the protocol; does not require consensus. labels Mar 24, 2020
@martinthomson
Copy link
Member

Yikes, that original name was a little hard to follow.

It strikes me that this code could be improved by consistently choosing either Idx or Index throughout. Though that might require some additional attention to line length if the longer (and better) option is chosen.

@afrind
Copy link
Contributor

afrind commented Mar 24, 2020

Broader question: do we still feel like the pseudocode is useful in general?

@kazu-yamamoto
Copy link
Contributor Author

kazu-yamamoto commented Mar 25, 2020

Let's suppose that nameIdx and nameIndex are the same.

The next question is how getNameIndex should behave?

The return value (nameIdx) is compared with staticTable.size. But it is also used for dynamic table. If my understanding is correct, the index spaces of the two table are independent. I cannot understand what is returned from getNameIndex.

Can getNameIndex return None? If nameIdx can be None, nameIdx <= staticTable.size is supposed to be True? If it cannot be None, it means that an index for the name can be always found, which, I think, is not realistic.

@afrind
Copy link
Contributor

afrind commented Apr 13, 2020

The code comes from a time when the index space was unified, and the first staticTable.size indexes were static and the rest were dynamic. And yes, getNameIndex can return "not found". I'll add some comments explaining this case and sanitize the pseudocode.

afrind added a commit that referenced this issue Apr 13, 2020
* Use the current terminology (insert count, base, encoder stream)
* Update to current design (modulo encoding of RIC)
* Normalize naming: index vs. idx
* Comments

Fixes #3544
@kazu-yamamoto
Copy link
Contributor Author

@afrind Thank you for your answer. Let's close this in favor of #3577.

afrind added a commit that referenced this issue May 19, 2020
* [qpack] Overhaul the pseudocode

* Use the current terminology (insert count, base, encoder stream)
* Update to current design (modulo encoding of RIC)
* Normalize naming: index vs. idx
* Comments

Fixes #3544

* Fix lint

* Martin's feedback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-qpack editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants