Skip to content

Conversation

@Diapolo
Copy link

@Diapolo Diapolo commented Apr 22, 2012

  • add some comments / expand some comments
  • move MyGetSpecialFolderPath() to another #ifdef WIN32 place, rename to GetSpecialFolderPath(), make fCreate default to true (could perhaps be removed and set to always true in the function) and remove fallbacks for SHGetSpecialFolderPathA() (I added an error message instead - this had been suggested in a former pull-request)
  • remove namespace fs = boost::filesystem; where fs was only used once to shorten the code

src/util.cpp Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale for changing the name of this link? It's fine as long as it points to the right file isn't it?

This does complicate matters, and we'll have to handle both possible names forever.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Luke suggested this at some time and as the application is named Bitcoin-Qt ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is but no one looks in the startup menu, and if they do they see 'Bitcoin' they can guess it's the UI. No need to fix what is not broken.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with reverting this change, but would like to know what others think to not revert revert :).

@Diapolo
Copy link
Author

Diapolo commented Apr 23, 2012

Rebased, only decision left is the rename of Bitcoin.lnk to Bitcoin-Qt.lnk. Please vote and I'll follow.

@Diapolo
Copy link
Author

Diapolo commented Apr 29, 2012

Revert to old Bitcoin autostart link (Bitcoin.lnk), so there should be no blocker in here anymore.

@Diapolo
Copy link
Author

Diapolo commented Apr 30, 2012

Rebased, removed most space changing stuff and removed the compiler-warning fix, which goes to a seperate pull-request.

@sipa
Copy link
Member

sipa commented May 3, 2012

ACK

@Diapolo
Copy link
Author

Diapolo commented May 5, 2012

Rebased, no code changes.

@jgarzik
Copy link
Contributor

jgarzik commented May 8, 2012

Why does this patch add a bunch of redundant initializations of large buffers?

BTW/hint: the commit message should answer this question... "utility functions cleanup / update" is too vague; it tells us nothing about why these changes were wanted/needed.

@Diapolo
Copy link
Author

Diapolo commented May 8, 2012

@jgarzik If an array has no initialization, the values are undefined, no? You are right about the commit message, it's one of my early pull-reqs ^^.

@jgarzik
Copy link
Contributor

jgarzik commented May 8, 2012

Correct. However, you will note that these undefined values are never accessed.

@Diapolo
Copy link
Author

Diapolo commented May 8, 2012

I'm willing to learn again, if the main devs don't want arrays to get initialized, I will remove that change.

@laanwj
Copy link
Member

laanwj commented May 9, 2012

In general we want variables to be initialized. But initializing large buffers can cause performance problems, I think the decision not to initialize them was conscious. I'd rather completely get rid of the buffers and replace them by a sensible string type, where possible.

(For example, add a function vstrprintf that takes a va_list like vsprintf, and use that everywhere instead of raw calls to snprintf and all the boilerplate around it)

…-> GetSpecialFolderPath(), set fCreate default to true and remove the fallback (not Win >= Vista compatible anyway) / remove namespace fs stuff where only used once / misc small changes'
@Diapolo
Copy link
Author

Diapolo commented May 9, 2012

Rebased and removed the char inits, only 1 change is left in FormatException().

@laanwj
Copy link
Member

laanwj commented May 9, 2012

ACK

jgarzik pushed a commit that referenced this pull request May 9, 2012
@jgarzik jgarzik merged commit 11b729d into bitcoin:master May 9, 2012
coblee pushed a commit to litecoin-project/litecoin that referenced this pull request Jul 17, 2012
suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017
Yet another PrivateSend refactoring/optimization - core part
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Jan 22, 2019
* Move requester.SendRequests() up before we release the node refs.

This way we don't have to take so many vNodes locks during
SendRequests().

* Remove unnecessary vNodes locks

* Add additional comments explaining the why we don't need to lock vNodes
when releasing refs and also, at times, when adding refs in the
requestManager.

and also tidy up the formatting in ThreadMessageHandler()

* This comment is actually not true

We don't have to keep the refs here all the way until the end
because the requestManager has its own set of refs as well, however,
it seems sensisble to do so anyway as a last step.

* Add DbgAssert() to ensure that we have a ref to the node already

and therefore don't need to lock vNodes.

* DbgAssert for nRefCount

nRefCount should always be greater than zero before we release a ref.
lateminer pushed a commit to lateminer/bitcoin that referenced this pull request Dec 25, 2019
d4a2c64 [Util] Remove a duplicate variable definition (warrows)

Pull request description:

  `nTimeSlotLength` was set twice to `15`. Line 182 and 192 of `chainparams.cpp`.

ACKs for top commit:
  random-zebra:
    utACK d4a2c64
  furszy:
    utACK d4a2c64

Tree-SHA512: 263c33f8d326840299ad8ae1848a46caabe5c687adf372dff80b2cd959df68acb15eca67e10fb7b1147c47796c50d684e569e7d5184a3603c7f402fd7111d66e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants