Make "annotate_ifdef_directives" script comply with line-width limits

Right now, our annotate_ifdef_directives script generates output that is wider than 80 columns. This is okay for now, when we're applying it by hand, but won't be okay in the long run, when we eventually want to have autostyle run before every commit.

changed milestone to %Tor: 0.4.2.x-final

Trac:
Parent Ticket: #31713 (moved)
Child Ticket(s): #31779 (moved)

added 042-should actualpoints::.6 component::core tor/tor milestone::Tor: 0.4.2.x-final owner::nickm parent::31713 priority::medium resolution::fixed reviewer::catalyst severity::normal sponsor::31-can status::closed type::defect labels

Trac:
Owner: N/A to nickm
Status: new to accepted

Branch at ticket31759; also fixes #31779 (moved). PR at https://github.com/torproject/tor/pull/1339 ; this can become needs_review once CI has passed.

Trac:
Actualpoints: N/A to .1

Trac:
Sponsor: N/A to Sponsor31-can
Status: accepted to needs_review

Trac:
Reviewer: N/A to catalyst

This pull request seems to contain code from #31338 (moved) as well. Was that intentional?

Oh dear, it is not. Hang on...

I've force-pushed a version with only the relevant commits. I'll keep an eye on CI.

(CI has passed)

Replying to nickm:

Branch at ticket31759; also fixes #31779 (moved). PR at https://github.com/torproject/tor/pull/1339 ; this can become needs_review once CI has passed. Thanks! This mostly looks good by inspection. I manually ran the script and verified that it produces the claimed result in the make autostyle commit.

I did make a comment on the pull request about how our stated (and enforced) limit seems to be 79 characters. Which one is correct? (I would say that 79 characters is better than 80, because of diff line prefixes, etc.)

(Also there seem to be multiple problems with running the make targets for various maintainer scripts from a separate build directory, but I should probably open new ticket(s) for that.)

I did make a comment on the pull request about how our stated (and enforced) limit seems to be 79 characters. Which one is correct? (I would say that 79 characters is better than 80, because of diff line prefixes, etc.)

This is actually a subtle issue in the annotate_ifdef_directives code: our lines are 79 characters long, not counting the terminating newline. The commented_line() function assumes that it gets a terminating newline in its input, and so uses 80 as the max line width.

But yeah, I agree this is not clear.

I could do any of these solutions:

• Document that commented_line() requires a fmt that ends with a newline, and that LINE_WIDTH includes the newline.
• Change the calls to commented_line() so that they do not have a newline in fmt, and change LINE_WIDTH to 79.
• ... something else?

Is there one that you think is best?

Replying to nickm:

I did make a comment on the pull request about how our stated (and enforced) limit seems to be 79 characters. Which one is correct? (I would say that 79 characters is better than 80, because of diff line prefixes, etc.)

This is actually a subtle issue in the annotate_ifdef_directives code: our lines are 79 characters long, not counting the terminating newline. The commented_line() function assumes that it gets a terminating newline in its input, and so uses 80 as the max line width.

But yeah, I agree this is not clear.

I could do any of these solutions:

• Document that commented_line() requires a fmt that ends with a newline, and that LINE_WIDTH includes the newline.
• Change the calls to commented_line() so that they do not have a newline in fmt, and change LINE_WIDTH to 79.
• ... something else?

Is there one that you think is best?

I was going to start suggesting the second option (have LINE_WIDTH, etc. count non-newline characters. Then I checked and realized that Python retains newlines in strings read from files, and writelines() doesn't add line separators. So now I'm thinking we should do the first option. We should also note that we're assuming universal newline mode (the Python default, I think? I only looked up it up in Python 3.), so if the OS uses something like CRLF line endings, the count is still valid.

This might be a minor thing: I just noticed that your changes produce unbalanced parentheses in the output (inside comments). Some of the old comments that it rewrote had differently-unbalanced parentheses. It would be nice if the script could produce balanced parentheses in the truncated output. I do sometimes use the balanced expression movement commands inside comments. Perhaps other people also do similarly? (Maybe the script could put the ellipsis inside the appropriate parentheses?)

Also I agree with teor's suggestion of adding a test case for the 79 vs 80 characters. That way we won't have to do quite so much convincing ourselves that the line width enforcement is correct. (Boundary condition tests are often good.)

Both are implemented by the same branch, which could use some minor revisions

Trac:
Status: needs_review to needs_revision

I've updated the branch and PR at ​https://github.com/torproject/tor/pull/1339 .

There are now tests, in the form that python's doctest module. I've opened a new ticket #31869 (moved) to run them automatically. To try them yourself, run "python -m doctest ./scripts/maint/annotate_ifdef_directives.py"

I've tried to document the 79/80 character issue and the newline handling more carefully.

I've added some code to produce balanced parentheses.

Predictably, adding tests found a bug in the negation-processing code: I had exchanged two characters in a regular expression. I added a fixup! commit to handle that.

I'll put this back in needs_review once CI has passed.

(I was not able to include a commit in this branch where I re-run autostyle, since it would cause conflicts with master. As part of review, please "make autostyle" and verify that the output looks reasonable.)

Trac:
Status: needs_revision to needs_review

I think the PR is missing at least 2 commits:

• the fixup for the bug discovered by the tests
• a commit that runs the self-test as part of "make check"

Did you forget to push?

Trac:
Status: needs_review to needs_revision

Oh dear. I had tried not to squash that fixup commit, but I think I squashed it by accident.

I'd like to handle running the self-test as part of "make check" as part of #31869 (moved), if that's okay.