How about we stop numbering spec steps in code?

[gterzian]

This is not a proposed change to code, rather to review practice.

I propose we stop numbering spec steps in the code, for the following reasons:

1. It's tedious,
2. Code implementing a step of the spec should be doing so obviously, without the need for numbering.
3. It's counterproductive, since like all comments, it gets out of date very quickly and makes understanding code actually harder after spec changes.

I propose that instead, besides adding a link to the relevant algorithm at the top of a function, we only add comments in the body when doing something that isn't obvious(or calls into some other part of the system), and then we comment not with a number but by actually writing it down in English, probably copy/pasting the actual line in the spec(the actual wording of a step doesn't change as often as its numbering).

Looking forward to your thoughts.

[pshaughn]

Spec wording instead of numbers does make sense, considering how often an editorial spec change renumbers everything below it without making any real change to the algorithm.

As for implementing steps "obviously", the spec often implicitly assumes variables are mutable, dynamically typed, and garbage-collected, and most of the non-trivial algorithms I touched had at least one non-obvious point in their implementation to line them up with the way Rust variables work. The average method that handles more than a few steps probably needs at least one internal comment.

[jdm]

I remain strongly in favour of comments that indicate all steps of algorithms based on my experience understanding code as a reviewer and reading code later. That being said, we used to paraphrase the spec we were implementing, then some members of the team pushed back and said that was redundant so we moved to numbered steps.

[pshaughn]

If every step is going to be commented, numbers are a less tedious way to do it than words. To mitigate out-of-dateness, maybe functions with numbered steps could have an additional link to the specific spec version the numbers came from, so when they go out of date the renumbering is easily observed?

[gterzian]

maybe functions with numbered steps could have an additional link to the specific spec version the numbers came from, so when they go out of date the renumbering is easily observed?

I think I would rather keep the status-quo, than start adding more precise information, in the sense that my point is that we should not aim for precise documentation of spec steps.

I agree that the numbering is useful during an initial review, and what I meant is that it does tend to get out of date after some time, which is not that big of a problem(it's not that hard to find the renumbered step), rather it does call into question whether we need the numbers in the code in the first place.

Example:

servo/components/script/fetch.rs

Line 220 in 7010178

// Step 4.1

This refers to Step 4.1 of I think https://fetch.spec.whatwg.org/#fetch-method

Now this seems to have become Step 9.3

However, That step is actually part of sub-steps for the "#process-response" task referred to as https://fetch.spec.whatwg.org/#ref-for-process-response%E2%91%A1 (the first ref to https://fetch.spec.whatwg.org/#process-response).

So I think it would be much clearer, and robust to numbering changes, to actually link to https://fetch.spec.whatwg.org/#process-response at

servo/components/script/fetch.rs

Line 211 in 7010178

fn process_response(&mut self, fetch_metadata: Result<FetchMetadata, NetworkError>) {

This link is actually absent from the current code, and then perhaps add a // If response is a network error at

servo/components/script/fetch.rs

Line 221 in 7010178

Err(_) => {

Instead of // Step 4.1.

So I'm not saying that the current // Step 4.1 is not precise enough, rather that in this case(and I think there are more, where we call into sub-sytems, do stuff over IPC and what not, without explaining what it does or how it relates to the spec) there is actually an important part of documentation missing, and that the out of date numbering provides some anchor but not that much.