Page MenuHomePhabricator

A pack of nits in net_processing.cpp
ClosedPublic

Authored by deadalnix on Jul 30 2017, 19:14.

Details

Summary

Mostly braces

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

CCulianu added a subscriber: CCulianu.

Visually reviewed (my eyes are getting sore now!!) -- looks good. No typos, error, etc that I can see and it compiled, tested, and tests pass.

FWIW: I disagree with the religious "put braces around everything" I think sometimes for really small if's, it's ok to do a one-liner, etc.. but I get that it's your project @deadalnix, and these are your standards, so I diffidently say this.

src/net_processing.cpp
3217 ↗(On Diff #1014)

LOL @ Core.

Why create a whole function-wide scope?

LOL. Yeah, good, get rid of it.

This revision is now accepted and ready to land.Aug 3 2017, 17:56
This revision was automatically updated to reflect the committed changes.

FWIW: I disagree with the religious "put braces around everything" I think sometimes for really small if's, it's ok to do a one-liner

Eventually, we'll get clang-tidy to do it automatically. Also I ran into a few case that are very confusing, like:

if (...) dosomething;
{
  LOCK(...);
  // ...
}

There are a few of these in the codebase, taking advantage of RAII to unlock the mutex. Very confusing.

Eventually, we'll get clang-tidy to do it automatically. Also I ran into a few case that are very confusing, like:

if (...) dosomething;
{
  LOCK(...);
  // ...
}

There are a few of these in the codebase, taking advantage of RAII to unlock the mutex. Very confusing.

Ouch. I agree the above is a trap. As are a lot of the ones you fixed which vastly improved readability (good work!).

But honestly don't you think something like:

if ( !fFlag ) vec += " ";

Or something equally trivial .. putting braces around it sort of makes the function longer and arguably doesn't add much in terms of readability as a result of that sacrifice (less fits on screen, etc).

Anyway this stuff isn't hugely important. Like I say -- it's your project and you get to set the standard.