Opened 3 years ago

Closed 3 years ago

#15585 closed enhancement (not a bug)

/src/util.c refactoring

Reported by: arcadiaq Owned by: arcadiaq
Priority: Very Low Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: refactoring, Tor
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I've tried to refactor the /util.c code as it seems to lack the unified codestyle and I tried to make it more readable and better looking, like adding the braces to every if block and so on.

The branch name is "fix_util" and I attach the patch generated via git format-patch master --stdout > fix_util.patch".

The original repository is https://gitweb.torproject.org/tor.git

Child Tickets

Attachments (1)

fix_util.patch (38.7 KB) - added by arcadiaq 3 years ago.

Download all attachments as: .zip

Change History (7)

Changed 3 years ago by arcadiaq

Attachment: fix_util.patch added

comment:1 Changed 3 years ago by arcadiaq

Owner: set to arcadiaq
Status: newassigned

comment:2 Changed 3 years ago by yawning

Hmm, there's nothing in the style guidelines forbidding this behavior, and leaving out braces for single line if statements is something done throughout the rest of the code, so I'm not seeing the point of this change.

Since it's cosmetic and isn't something that's consistent with the rest of the codebase I'm inclined to NACK this.

comment:3 in reply to:  2 ; Changed 3 years ago by arcadiaq

Replying to yawning:

Hmm, there's nothing in the style guidelines forbidding this behavior, and leaving out braces for single line if statements is something done throughout the rest of the code, so I'm not seeing the point of this change.

Since it's cosmetic and isn't something that's consistent with the rest of the codebase I'm inclined to NACK this.

Well, that's right, but the readability of the code is way better this way. Such constructions as
if (condition) statement
are pretty ugly and I can't see why they should remain in the code. The style guidelines are too vague and they allow very strange constructions to remain in the code. The cosmetic changes should keep the code simple and fancy and get rid of the chaos in the source code.

Last edited 3 years ago by arcadiaq (previous) (diff)

comment:4 in reply to:  3 Changed 3 years ago by yawning

Priority: normaltrivial

Replying to arcadiaq:

Hmm, there's nothing in the style guidelines forbidding this behavior, and leaving out braces for single line if statements is something done throughout the rest of the code, so I'm not seeing the point of this change.

Since it's cosmetic and isn't something that's consistent with the rest of the codebase I'm inclined to NACK this.

Well, that's right, but the readability of the code is way better this way. Such constructions as
if (condition) statement
are pretty ugly and I can't see why they should remain in the code. The style guidelines are too vague and they allow very strange constructions to remain in the code. The cosmetic changes should keep the code simple and fancy and get rid of the chaos in the source code.

if (condition) statement can be ugly if the statement is more than something extremely trivial.

  if (condition)
    statement;

Is a common and well established style guideline for a lot of large projects (See style(9) if you are a BSD person, "Linux kernel coding style" if you are a Linux person).

Anyway, it's up to nickm, but my opinion still stands for the most part (the two lines where the new line is missing could be changed to add a newline, though I think if (TOR_ISODIGIT(*cp)) ++cp; is perfectly understandable).

comment:5 Changed 3 years ago by nickm

Milestone: Tor: 0.2.8.x-final

comment:6 Changed 3 years ago by nickm

Resolution: not a bug
Status: assignedclosed

I think that I agree with yawning and this is mostly a don't-take. If we did decide to use braces around all statements, we'd probably want to do it in an automated way (using coccinelle maybe) to be sure of not messing it up

Note: See TracTickets for help on using tickets.