Opened 4 weeks ago

Closed 2 weeks ago

#31759 closed defect (fixed)

Make "annotate_ifdef_directives" script comply with line-width limits

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 042-should
Cc: Actual Points: .6
Parent ID: #31713 Points:
Reviewer: catalyst Sponsor: Sponsor31-can

Description

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.

Child Tickets

TicketStatusOwnerSummaryComponent
#31779closednickmannotate_ifdef_directives: avoid double-negationCore Tor/Tor

Change History (27)

comment:1 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 4 weeks ago by nickm

Actual Points: .1

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

comment:3 Changed 4 weeks ago by nickm

Sponsor: Sponsor31-can
Status: acceptedneeds_review

comment:4 Changed 3 weeks ago by asn

Reviewer: catalyst

comment:5 Changed 3 weeks ago by teor

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

comment:6 Changed 3 weeks ago by nickm

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

comment:7 Changed 3 weeks ago by nickm

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

comment:8 Changed 3 weeks ago by nickm

(CI has passed)

comment:9 in reply to:  2 Changed 3 weeks ago by catalyst

Replying to nickm:

Branch at ticket31759; also fixes #31779. 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.)

comment:10 Changed 3 weeks ago by 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?

comment:11 in reply to:  10 Changed 3 weeks ago by catalyst

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.)

comment:12 Changed 3 weeks ago by catalyst

Status: needs_reviewneeds_revision

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

comment:13 Changed 3 weeks ago by nickm

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 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.

comment:14 Changed 3 weeks ago by nickm

(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.)

comment:15 Changed 3 weeks ago by nickm

Status: needs_revisionneeds_review

comment:16 Changed 3 weeks ago by teor

Status: needs_reviewneeds_revision

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?

comment:17 Changed 3 weeks ago by nickm

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, if that's okay.

comment:18 Changed 3 weeks ago by nickm

It looks like the fixup was squashed into the "remove some cases of double negation" commit. Here is the code diff between the old version of that commit, and the new one.

     expr = expr.strip()
     # See whether we match !(...), with no intervening close-parens.
-    m = re.match(r'^!\s*\(([^\)*])\)$', expr)
+    m = re.match(r'^!\s*\(([^\)]*)\)$', expr)
     if m:
         return m.group(1)
-    # See whether we match !defined(...), with no intervening close-parens.
-    m = re.match(r'^!\s*(defined\([^\)]*\))$', expr)
+
+
+    # See whether we match !?defined(...), with no intervening close-parens.
+    m = re.match(r'^(!?)\s*(defined\([^\)]*\))$', expr)
     if m:
-        return m.group(1)
+        if m.group(1) == "!":
+            prefix = ""
+        else:
+            prefix = "!"
+        return prefix + m.group(2)
 
     return "!(%s)" % expr

comment:19 Changed 3 weeks ago by teor

Here's the fixup on GitHub, it also contains some doctests:
https://github.com/torproject/tor/commit/704c23ca01e58fa8ba60c9a453475b404c5d2d0d

comment:20 Changed 3 weeks ago by nickm

(At catalyst's request, I've added one more doctest, to catch the 80-column line width case.)

comment:21 Changed 3 weeks ago by nickm

Status: needs_revisionneeds_review

comment:22 in reply to:  20 Changed 3 weeks ago by catalyst

Replying to nickm:

(At catalyst's request, I've added one more doctest, to catch the 80-column line width case.)

Thanks! These changes look good.

I left a minor comment on the pull request about a typo in a comment. Feel free to set to merge_ready once that's fixed.

comment:23 Changed 3 weeks ago by nickm

I've fixed the typo, and squashed the branch. I'll put this in merge_ready once CI has passed.

comment:24 Changed 3 weeks ago by nickm

Actual Points: .1.6
Keywords: asn-merge added
Status: needs_reviewmerge_ready

comment:25 Changed 2 weeks ago by asn

Keywords: asn-merge removed
Status: merge_readyneeds_information

Merged! However the child ticket is still open, so I'm leaving this also open until we can close it.

comment:26 Changed 2 weeks ago by asn

Status: needs_informationmerge_ready

comment:27 Changed 2 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

This branch also contains the child ticket fixes.

Note: See TracTickets for help on using tickets.