Opened 5 months ago

Closed 5 months ago

#33349 closed enhancement (fixed)

Small changes to scheduler comments and state changes

Reported by: opara Owned by:
Priority: Medium Milestone: Tor: 0.4.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-sched
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

I have attached a patch that tries to make some of the comments in scheduler.c easier to follow, such as the comments that reference the old channels_waiting_for_cells and channels_waiting_to_write lists that were removed in commit 3530825c5. It also simplifies a couple of the channel state changes.

Child Tickets

Attachments (2)

ticket33349.patch (5.9 KB) - added by opara 5 months ago.
addendum-1.patch (957 bytes) - added by opara 5 months ago.

Download all attachments as: .zip

Change History (13)

Changed 5 months ago by opara

Attachment: ticket33349.patch added

Changed 5 months ago by opara

Attachment: addendum-1.patch added

comment:1 Changed 5 months ago by dgoulet

Keywords: tor-sched added
Milestone: Tor: 0.4.4.x-final
Reviewer: dgoulet
Status: newneeds_review

We'll need a PR so our CI runs it because there are code changes.

@opara, if you can't provide one (via Github for now), let me know, I'll create a branch with your patch for it.

comment:2 Changed 5 months ago by opara

comment:3 Changed 5 months ago by teor

Status: needs_reviewneeds_revision

Looks like CI failed, let us know if you need help fixing the issues.

comment:4 Changed 5 months ago by opara

It seems to be upset about the changes/ticket33349 file using the header "Code simplifications and refactoring", which your doc/HACKING/CodingStandards.md documentation says I should use.

comment:5 Changed 5 months ago by opara

Oops forgot to include the error:

./changes/ticket33349:

	Unrecognized header: 'Code simplifications and refactoring'

make[1]: *** [check-changes] Error 1

comment:6 Changed 5 months ago by nickm

Apparently the lintChanges script only accepts "Code simplification and refactoring" [with no s at the end of "simplification"]. I'll edit the script to be a bit more tolerant, since the sortChanges.py script knows how to clean that one up.

comment:7 Changed 5 months ago by dgoulet

Status: needs_revisionmerge_ready

lgtm!

@opara, I've put in a comment in the PR, doesn't stop the merge of this patch but I would love it if you could confirm what I said there? Thanks!

(I'll merge once I get a ACK from opara).

comment:8 Changed 5 months ago by opara

Thanks, if you'd like I can update the header to remove the extra 's'. And @dgoulet I added a reply to the PR, if there's any way I can improve the code comment that you commented on let me know. I don't want to change anything while this ticket is in the 'merge_ready' state, but I can still change something if anyone would like.

comment:9 in reply to:  8 ; Changed 5 months ago by dgoulet

Replying to opara:

Thanks, if you'd like I can update the header to remove the extra 's'. And @dgoulet I added a reply to the PR, if there's any way I can improve the code comment that you commented on let me know. I don't want to change anything while this ticket is in the 'merge_ready' state, but I can still change something if anyone would like.

Nice thanks! And yes, just push a fixup commit for the removal of 's' and we'll be on our way to merge this.

Thanks!

comment:10 in reply to:  9 Changed 5 months ago by opara

Replying to dgoulet:

Nice thanks! And yes, just push a fixup commit for the removal of 's' and we'll be on our way to merge this.

I posted a fixup commit, but I'm not sure if you want me to squash it as well (or if you do that yourself so that you can audit changes better). If you want me to squash them let me know.

comment:11 Changed 5 months ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged thanks!

Note: See TracTickets for help on using tickets.