check-spaces.pl should check spaces after a comma when in functions.
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.
Trac:
Username: ewong
Activity
changed milestone to %Tor: unspecified
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/'-
please note.. due to confusion with rebasing and merging, I decided to create a new PR.
https://github.com/ewongbb/tor/tree/chkspace
Trac:
Username: ewong 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 };Trac:
Status: needs_review to needs_revision-
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.
Trac:
Username: 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
Trac:
Username: ewong 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.
-
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:
- changed indent where I think would be proper (though I probably missed quite a few)
- 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]
Trac:
Username: ewong Thanks for your work on this.
There's one last thing you need to fix:
- 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: 2. Merge this patch to master, and fix any spacing issues it identifies in new code
Trac:
Status: needs_review to needs_revision-
thanks teor!
there were a few bitrotted code, so I did the following: https://github.com/ewongbb/tor/tree/chkspace_f2
Trac:
Username: ewong 
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_033in 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?-
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_033in 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.
Trac:
Username: ewong moved to tpo/core/tor#23500 (closed)