Opened 11 months ago

Closed 8 months ago

#32610 closed enhancement (wontfix)

Add macro spacing checks to "make check-spaces"

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 043-deferred
Cc: nickm Actual Points: 0.4
Parent ID: #32522 Points: 0.2
Reviewer: nickm Sponsor:

Description

Let's check macro spacing like we check other keyword spacing.

And let's check for else {.

Child Tickets

Change History (13)

comment:1 Changed 11 months ago by teor

Status: assignedneeds_review

See my PR:

I ended up implementing:

  • check else {
  • check all the preprocessor directives we currently use
  • check for no spaces and multiple spaces (in cases where that makes sense)

comment:2 Changed 11 months ago by teor

Actual Points: 0.20.4

comment:3 Changed 11 months ago by teor

Status: needs_reviewneeds_revision

I need to revise this patch to check win32/orconfig.h in more places.
(I think at least coccinelle.)

comment:4 Changed 11 months ago by nickm

(Before we spend too much time on this, let's also make sure it's something that our code autoformatter won't take care of for us, so that the effort isn't redundant)

comment:5 in reply to:  4 Changed 11 months ago by teor

Status: needs_revisionneeds_review

I fixed the patch so all 4 lists of Tor C files are consistent.

Replying to nickm:

(Before we spend too much time on this, let's also make sure it's something that our code autoformatter won't take care of for us, so that the effort isn't redundant)

I think we're still some time away from automated C formatting. We should trial make autostyle first, see #31713. I think I finally fixed the last few instances of #31891 on Monday.

I discovered these issues during #32522, I don't expect to spend any more time on check-spaces. It is an improvement, and it shouldn't take much time to review.

I suggest we merge now, and then remove redundant (or conflicting) parts of check-spaces when we implement C autoformatting?

comment:6 Changed 11 months ago by teor

Possibly conflicting with #32613.

comment:7 Changed 11 months ago by nickm

Status: needs_reviewneeds_revision

The code looks plausible, but let's merge this after #32613, so we can add tests for the new failure cases here.

comment:8 Changed 10 months ago by teor

Keywords: network-team-roadmap-november removed

Sponsor 31 is over, this isn't a roadmap task any more.

comment:9 Changed 10 months ago by teor

I split the comments out of the final commit and pushed them to master. (They're unrelated to the rest of this branch.)

So we'll need to deal with a merge conflict, if we ever push these changes.

Last edited 10 months ago by teor (previous) (diff)

comment:10 Changed 9 months ago by nickm

Keywords: 043-deferred added

All 0.4.3.x tickets without 043-must, 043-should, or 043-can are about to be deferred.

comment:11 Changed 9 months ago by nickm

Milestone: Tor: 0.4.3.x-finalTor: unspecified

comment:12 Changed 9 months ago by gaba

Sponsor: Sponsor31-can

No more sponsor 31. All this tickets remained open after sponsor 31 ended.

comment:13 Changed 8 months ago by teor

Resolution: wontfix
Status: needs_revisionclosed

Probably obsoleted by the C autostyle changes.

Note: See TracTickets for help on using tickets.