Opened 10 months ago

Closed 9 months ago

#32211 closed task (implemented)

write description of subsystem initialization/shutdown architecture

Reported by: catalyst Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-september, s31-docs
Cc: teor, nickm Actual Points: .2
Parent ID: #29215 Points:
Reviewer: catalyst Sponsor: Sponsor31-must

Description


Child Tickets

Change History (9)

comment:1 Changed 10 months ago by catalyst

Owner: set to nickm
Status: newassigned

comment:2 Changed 9 months ago by catalyst

Type: defecttask

these are tasks not defects

comment:3 Changed 9 months ago by nickm

See branch ticket32211 for a small first draft here. I imagine that more documentation is probably desirable; any guidance would be welcome. PR at https://github.com/torproject/tor/pull/1520

comment:4 Changed 9 months ago by nickm

Actual Points: .2
Status: assignedneeds_review

comment:5 Changed 9 months ago by asn

Reviewer: catalyst

comment:6 in reply to:  3 ; Changed 9 months ago by catalyst

Replying to nickm:

See branch ticket32211 for a small first draft here. I imagine that more documentation is probably desirable; any guidance would be welcome. PR at https://github.com/torproject/tor/pull/1520

Thanks! Looks good so far. What areas do you think need more work?

We might want to explain why we would want to add new fields in the middle of subsys_fns_t.

In initialization.dox, maybe move the reference to subsys_fns_t closer to the beginning of that paragraph? Maybe like

subsys_fns_t describes a subsystem and a set of functions [...] To define a subsystem...

As a minor style nit, do we need to use backslashes for Doxygen commands in that places you use them here? I would like us to move towards more consistently using at-signs for Doxygen commands.

comment:7 in reply to:  6 ; Changed 9 months ago by nickm

Replying to catalyst:

Replying to nickm:

See branch ticket32211 for a small first draft here. I imagine that more documentation is probably desirable; any guidance would be welcome. PR at https://github.com/torproject/tor/pull/1520

Thanks! Looks good so far. What areas do you think need more work?

I don't know, TBH. I'm hoping that there will be questions I can answer more about.

We might want to explain why we would want to add new fields in the middle of subsys_fns_t.

I don't understand -- as opposed to at the end of it? Or why we'd add new fields at all?

In initialization.dox, maybe move the reference to subsys_fns_t closer to the beginning of that paragraph? Maybe like

subsys_fns_t describes a subsystem and a set of functions [...] To define a subsystem...

Sounds good.

As a minor style nit, do we need to use backslashes for Doxygen commands in that places you use them here? I would like us to move towards more consistently using at-signs for Doxygen commands.

I'm okay trying to remember to use @ signs in the future, but I'd rather not adopt a rule forcing them without tooling to convert and enforce. Does that sound okay?

comment:8 in reply to:  7 Changed 9 months ago by catalyst

Replying to nickm:

Replying to catalyst:

We might want to explain why we would want to add new fields in the middle of subsys_fns_t.

I don't understand -- as opposed to at the end of it? Or why we'd add new fields at all?

Yes, as opposed to at the end. Hm, given that this is justifying asking people to use named initializers, maybe we should also say that they help with code readability when many pointers need to be null? Also, I'm guessing that sometimes there are reasons to keep a logical grouping of fields together? (I'm pretty sure we're not optimizing for storage layout here given the existing order of fields in the structure.)

As a minor style nit, do we need to use backslashes for Doxygen commands in that places you use them here? I would like us to move towards more consistently using at-signs for Doxygen commands.

I'm okay trying to remember to use @ signs in the future, but I'd rather not adopt a rule forcing them without tooling to convert and enforce. Does that sound okay?

Yes. I'm happy if we adopt a guideline (not requirement) to prefer at-signs for Doxygen commands in new Doxygen comments.

comment:9 Changed 9 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Okay. I've updated and merged this branch. Thanks!

Note: See TracTickets for help on using tickets.