Opened 3 weeks ago

Last modified 4 days ago

#32522 needs_revision task

Create better tooling for canonical tor header includes

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-november
Cc: nickm Actual Points: 5
Parent ID: #31851 Points: 1
Reviewer: nickm Sponsor: Sponsor31-can

Description

When we fix includes, we could also:

  • standardise the whitespace
  • sort into a consistent order
  • remove duplicates

Eventually, we should try to find unused headers, but that seems like a separate ticket. (But we need to do this task first.)

Child Tickets

TicketStatusOwnerSummaryComponent
#32609closedteorImprove practracker unit tests, and run them in "make check" and pre-commitCore Tor/Tor
#32610needs_revisionteorAdd macro spacing checks to "make check-spaces"Core Tor/Tor
#32613closednickmSimple tests for checkSpaces.plCore Tor/Tor
#32655assignedteorTry finding unused includes by compiling without each includeCore Tor/Tor

Change History (7)

comment:1 Changed 3 weeks ago by teor

Actual Points: 0.8

I have a draft script, it's harder than I thought. I need to move the comments along with the headers/private defines they belong to.

comment:2 Changed 9 days ago by teor

Actual Points: 0.84

There's still some work to do here, but I should have a PR ready for review soon. I found a lot of interesting bugs and inconsistencies along the way, see the child tickets.

comment:3 Changed 5 days ago by teor

Actual Points: 45
Reviewer: nickm
Status: assignedneeds_review

Here's my draft pull request:

And here's what the code actually ends up doing:

  • better canonical header paths
  • delete unused PRIVATE, INTERNAL, and EXPOSE defines
  • delete duplicate includes
  • standardise whitespace (conforming to make check-spaces, plus extra fixes)
  • add command-line arguments to control which files get which fixes

comment:4 Changed 4 days ago by nickm

Hm. This looks like a complete rewrite to me. That's okay, but I'm going to have to make a few general comments before I move in to a line-by-line review. I hope that's okay.

1) The changes made by this file look good, and the improved documentation and code structure is nice.

2) I think this is the kind of rewrite where we want to have tests now. Do you think tests are in order here?

3) Do you think that we should standardize on one of PRIVATE, INTERNAL, or EXPOSE? Alternatively, do you think we should document the difference between them?

4) Can/should we reuse python's logging framework rather than rolling our own set of warning/error reporting functions?

5) When using "global", please make sure that you're actually modifying the variable. It isn't necessary to say "global" when you're only reading the variable.

6) This branch uses several different new maps, with new semantics. Would it make sense to turn them into one or more classes? If not, we should at least have an overview listing all of them and what they're for.

7) Does is really make sense to have "current_file" be a global? Is there some more OO approach that would make the code cleaner? (Obviously we shouldn't do that if it makes the code uglier.)

7) Consider running this script through a python style checker, if you haven't done so already; it usually catches a few things when I remember to do that.

8) I don't think that we should _remove_ any normalizations that this script does, but before we add any more normalizations, we should be sure that we aren't replicating work that our chosen code styling tool can already do for us.

Once we've decided what to do with the above, I can start on a line-by-line review.

comment:5 Changed 4 days ago by nickm

Status: needs_reviewneeds_revision

comment:6 in reply to:  4 Changed 4 days ago by teor

Replying to nickm:

Hm. This looks like a complete rewrite to me. That's okay, but I'm going to have to make a few general comments before I move in to a line-by-line review. I hope that's okay.

1) The changes made by this file look good, and the improved documentation and code structure is nice.

2) I think this is the kind of rewrite where we want to have tests now. Do you think tests are in order here?

Yes, and the script can be re-targeted at a test directory using its command-line options.

3) Do you think that we should standardize on one of PRIVATE, INTERNAL, or EXPOSE? Alternatively, do you think we should document the difference between them?

Yes, probably PRIVATE. I can make this change, and then modify the script.

4) Can/should we reuse python's logging framework rather than rolling our own set of warning/error reporting functions?

Probably, if we can work out how to get the current file context into it.

5) When using "global", please make sure that you're actually modifying the variable. It isn't necessary to say "global" when you're only reading the variable.

I think this is tied up with 4) and 7).

6) This branch uses several different new maps, with new semantics. Would it make sense to turn them into one or more classes? If not, we should at least have an overview listing all of them and what they're for.

Probably one or two classes, implemented with an internal map.

7) Does is really make sense to have "current_file" be a global? Is there some more OO approach that would make the code cleaner? (Obviously we shouldn't do that if it makes the code uglier.)

I'll see if python's logging has some kind of context argument.

7) Consider running this script through a python style checker, if you haven't done so already; it usually catches a few things when I remember to do that.

Do we have a recommended python style checker? Should we standardise on one, and start moving our scripts to it?

8) I don't think that we should _remove_ any normalizations that this script does, but before we add any more normalizations, we should be sure that we aren't replicating work that our chosen code styling tool can already do for us.

I don't expect to add any more normalizations, my next step is #32655, which a styling tool definitely can't do.

Once we've decided what to do with the above, I can start on a line-by-line review.

comment:7 Changed 4 days ago by teor

I think I'd rather revise first, and then have the review.

Note: See TracTickets for help on using tickets.