Opened 2 months ago

Last modified 5 weeks ago

#32610 needs_revision enhancement

Add macro spacing checks to "make check-spaces"

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: nickm Actual Points: 0.4
Parent ID: #32522 Points: 0.2
Reviewer: nickm Sponsor: Sponsor31-can

Description

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

And let's check for else {.

Child Tickets

Change History (9)

comment:1 Changed 8 weeks 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 8 weeks ago by teor

Actual Points: 0.20.4

comment:3 Changed 8 weeks 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 8 weeks 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 8 weeks 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 8 weeks ago by teor

Possibly conflicting with #32613.

comment:7 Changed 7 weeks 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 5 weeks ago by teor

Keywords: network-team-roadmap-november removed

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

comment:9 Changed 5 weeks 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 5 weeks ago by teor (previous) (diff)
Note: See TracTickets for help on using tickets.