Opened 13 months ago

Last modified 9 months ago

#23500 needs_revision enhancement

check-spaces.pl should check spaces after a comma when in functions.

Reported by: ewong Owned by:
Priority: Low Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Trivial Keywords: code-style, review-group-24
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

right now, running |make check-spaces| ignores instances such
that there are no spaces after commas.

i.e.
tt_int_op(1,OP_EQ, r);

for consistency's sake, there should be a space after the comma.

Child Tickets

Change History (22)

comment:2 Changed 13 months ago by nickm

Milestone: Tor: 0.3.3.x-final
Status: newneeds_review

comment:3 Changed 13 months ago by ewong

Summary: check-spaces.pl doesn't check for spaces not being after a comma in functionscheck-spaces.pl should check spaces after a comma when in functions.

comment:4 Changed 13 months ago by teor

Keywords: code-style added
Type: defectenhancement

Thanks for this patch, we'll get to it soon.

When we have the final version of the script, it would also be helpful to do a mass replace patch that makes the script pass on the current codebase.

comment:5 Changed 13 months ago by ewong

Do you want me to do this in the same pr or a different one?

comment:6 in reply to:  5 Changed 13 months ago by teor

Replying to ewong:

Do you want me to do this in the same pr or a different one?

Same pr and branch is fine, but a different commit.

Usually we do mass replaces using a script, and then put that script in the commit.
That way someone can verify that commit only contains the results of the script.

For example, a script for this might be something like:
sed 's/,([^ ])/, \1/'

comment:7 Changed 13 months ago by ewong

please note.. due to confusion with rebasing and merging, I decided to
create a new PR.

https://github.com/ewongbb/tor/tree/chkspace

comment:8 Changed 13 months ago by teor

Status: needs_reviewneeds_revision

I'm not sure if we want to add spaces after integers in structure initialisations, or in MOCK_IMPL declarations. But it seems ok to me. I'll leave it to nickm to make the final call.

Also, have you checked that mass replace passes make check-spaces?

I think this line is too long, and there may be others:

        static char nil_bytes[16] = { [0]=0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };

comment:9 in reply to:  8 Changed 13 months ago by ewong

Replying to teor:
ve you checked that mass replace passes make check-spaces?

I think this line is too long, and there may be others:

        static char nil_bytes[16] = { [0]=0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 };

ah right. Point taken.

comment:10 Changed 13 months ago by ewong

as it stands, the find.. command is getting difficult since I have yet to discover
a single command line which checks if the additional ','->', ' change will increase
the line length.. if so, it changes the line and then wraps the line near the end;
but that affects a bunch load of other lines.

I'm starting to feel like just going through the list of files and changing
it manually will make it 'easier'. might anyone have any advice?
thanks

comment:11 in reply to:  10 Changed 13 months ago by teor

Replying to ewong:

as it stands, the find.. command is getting difficult since I have yet to discover
a single command line which checks if the additional ','->', ' change will increase
the line length.. if so, it changes the line and then wraps the line near the end;
but that affects a bunch load of other lines.

I'm starting to feel like just going through the list of files and changing
it manually will make it 'easier'. might anyone have any advice?
thanks

A mass replace script in one commit, then a few manual fixes in the next commit is fine.

comment:12 Changed 13 months ago by ewong

This is my attempt at a 'mass change' https://github.com/ewongbb/tor/tree/chkspace_f

I had attempted to do a mass change with a script but the amount of time trying to
figure out the code which should be changed compared to those that shouldn't
would've been better used on reading and looking for the general instances
that apply. So I apologize as this 'mass change' was pretty much a manual process.

Do note I took a few liberties at the following:

1) changed indent where I think would be proper (though I probably missed quite

a few)

2) reformatted some lines to make it less 'Wide'

As such some caveats:
a) haven't touched C code(and even then, I was just a beginner)
b) I'm not familiar with the tor code and didn't want to do too much damage,

particularly with some whitespace which I think should be there but didn't
change. i.e. if conditions with all the conditions whitespaceless..

if (i=0;i<10;i++) {... } [no, this isn't from the code.. just an example]

comment:13 Changed 13 months ago by ewong

Status: needs_revisionneeds_review

comment:14 Changed 13 months ago by teor

Status: needs_reviewneeds_revision

Thanks for your work on this.

There's one last thing you need to fix:

  1. Put all the changes to check-spaces.pl in one commit. In the latest branch, the script is changed in the first and third commits.

Once that's done, I'll send this to the release manager and see if they are happy with the manual mass change.
We can use diff's ignore whitespace option to check that only whitespace was modified.
But we'll still need to manually confirm that none of the whitespace was significant.

Then, after final review, we need to:

  1. Merge this patch to master, and fix any spacing issues it identifies in new code

comment:15 Changed 13 months ago by ewong

thanks teor!

there were a few bitrotted code, so I did the following:
https://github.com/ewongbb/tor/tree/chkspace_f2

comment:16 Changed 12 months ago by ewong

Status: needs_revisionneeds_review

comment:17 Changed 12 months ago by nickm

Keywords: review-group-24 added

review-group-24 is now open.

comment:18 in reply to:  14 ; Changed 12 months ago by nickm

Hm. Did you use a script to add the missing spaces, or did you do it by hand?

Ideally, there should be:

  • one commit that changes checkSpace.pl and nothing else.
  • a script that we can run on master to add the missing spaces when we merge. This way, we don't get any conflicts, and it's easy to verify that the script does only what it claims. Maybe something like this?
    perl -i -pe 's/,(\S)/, $1/g' src/{common,or,ext,test,tools}/*.[ch]
    
  • And then we can just run wrap the lines by hand.

I've extracted the script as its own commit in a branch ticket23500_033 in my public repository at https://gitweb.torproject.org/nickm/tor.git . If it looks okay, and somebody confirms the steps above, I can merge it and do the rest.

Also, is there a reason to have the checkSpaces script check for [\w|\d]+ instead of just \S?

comment:19 Changed 12 months ago by nickm

Status: needs_reviewneeds_revision

comment:20 in reply to:  18 Changed 12 months ago by ewong

Replying to nickm:

Hm. Did you use a script to add the missing spaces, or did you do it by hand?

I did it by hand.

Ideally, there should be:

  • one commit that changes checkSpace.pl and nothing else.
  • a script that we can run on master to add the missing spaces when we merge. This way, we don't get any conflicts, and it's easy to verify that the script does only what it claims. Maybe something like this?
    perl -i -pe 's/,(\S)/, $1/g' src/{common,or,ext,test,tools}/*.[ch]
    
  • And then we can just run wrap the lines by hand.

I've extracted the script as its own commit in a branch ticket23500_033 in my public repository at https://gitweb.torproject.org/nickm/tor.git . If it looks okay, and somebody confirms the steps above, I can merge it and do the rest.

Also, is there a reason to have the checkSpaces script check for [\w|\d]+ instead of just \S?

My regex-fu is basic at the best, so that's what I came up with. I had a major
issue with figuring out how to get sed to regex properly.

sorry for the mess.

comment:21 Changed 12 months ago by nickm

I've played with this some more, and run into trouble: The patch not only detects ,s inside of code, but also commas inside of comments and certain multiline strings.

comment:22 Changed 9 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: unspecified
Priority: MediumLow

I don't feel too strongly that the comma usage detected here actually _is_ bad style, so since the patch isn't working, I'm throwing it back into unspecified.

Note: See TracTickets for help on using tickets.