little changes / updates in the code #1136

Diapolo commented at 2:49 PM on April 22, 2012: none

cleanup some unnecessary spaces
updated a comment
fix a compiler warning because of missing {} (https://github.com/bitcoin/bitcoin/pull/1136/files#L5R1588 and following lines)
remove typedef FILE element_type; from serialize.h as this is used nowhere

in src/util.cpp:None in 4fcf9ac90d outdated

 659 | @@ -660,7 +660,7 @@ string EncodeBase64(const string& str)
 660 |      int mode = 0;
 661 |      int left = 0;
 662 |  
 663 | -    while (1)
 664 | +    loop

laanwj commented at 3:09 PM on April 22, 2012:

Why do we have this loop macro? (defined as for(;;)). It's pretty confusing and doesn't add anything to the simple C syntax, does it?

Edit: this is just a general comment, not specific for your pull request...

Diapolo commented at 3:16 PM on April 22, 2012:

I'm fine with removing it everywhere and usw while (1), but it was in and so I used it.

laanwj commented at 3:25 PM on April 22, 2012:

No need to remove it everywhere, but also no need to use it in new places IMO.

Diapolo commented at 3:28 PM on April 22, 2012:

I tried to not rename any variables as I understood it makes little sense ^^, but if we have a #define for something we should use it or throw it out alltogether IMHO.

laanwj commented at 3:34 PM on April 22, 2012:

It's also possible to deprecate it for new code and phase it out slowly (as another Satoshi-ism). We don't like change-stuff-all-over-the-place commits.

Diapolo commented at 3:36 PM on April 22, 2012:

Will revert ...

Diapolo commented at 3:42 PM on April 22, 2012: none

Reverted to while (1) instead of loop, but as laanwj said the use of loop should perhaps be deprecated, or loop should be removed (not with this pull request).

sipa commented at 3:49 PM on April 22, 2012: member

Most changes look like minor code style changes, or personal preferences ("char* x" vs "char *x", for example). Not sure that's worth the trouble of possibly conflicting with other changes.

laanwj commented at 3:53 PM on April 22, 2012: member

Well the changes to net.cpp and serialize.h, and the comment update in init.cpp make sense.

Removing/adding spaces on the other hand (especially at the end of the line), meh...

jgarzik commented at 4:04 PM on April 22, 2012: contributor

@sipa agreed

Diapolo commented at 5:11 PM on April 22, 2012: none

I saw you guys removing left-over spaces, too ... so what's wrong with beeing thorough?

cleanup spaces, fix a compiler warning and remove a dead typedef 0b1a0607ca

luke-jr commented at 5:17 PM on April 22, 2012: member

Removing spaces when you're modifying the line already is fine... removing it with no other changes, on the other hand...

Diapolo commented at 5:26 PM on April 22, 2012: none

It isn't even worth the discussion, I'm not searching for those parts in the code, but while merging commits I see those things with BeyondCompare. The need not to be in and so it should be simply okay to change this whithout any discussion. I won't remove any line-breaks and reverted that!

Diapolo commented at 5:26 PM on April 29, 2012: none

Will this get merged or shall I chery-pick the 3 changes and we leave unused spaces all over the code ;)?

jgarzik commented at 7:00 PM on April 30, 2012: contributor

Basically this is a whole bunch of unrelated changes, lumped into a single commit.

Even if we're talking about two one-line changes, consider putting such changes each in a separate commit if they are logically unrelated.

In particular, trim trailing whitespace should never be mixed in with other changes. I created a separate pull request doing the trimming, see #1170.

Closing, but you are welcome to resubmit split-up code changes.

jgarzik closed this on Apr 30, 2012

Diapolo commented at 11:32 PM on April 30, 2012: none

I'm fine with splitting this up into several pull-requests, if that is the wished procedure :).

laanwj commented at 6:51 AM on May 1, 2012: member

Mind that @jgarzik is talking about multiple commits, not necessarily multiple pull requests. Multiple pull requests are good for completely unrelated things, but don't feel obligated to make a separate pull request for each warning message, for example.

Diapolo commented at 9:57 AM on May 1, 2012: none

Thanks for clarifiying even further, I don't feel disabused, but it helps me with helping the project!

lateminer referenced this in commit 06b3bc5536 on Jan 22, 2019

lateminer referenced this in commit 0f92bf8e17 on Dec 25, 2019

DrahtBot locked this on Sep 8, 2021