small util.cpp/h changes #1134

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

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

in src/util.cpp:None in 1cd6d29b9a outdated

1084 | +boost::filesystem::path StartupShortcutPath(bool fLegacy)
1085 | +{
1086 | +    if (fLegacy)
1087 | +        return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin.lnk";
1088 | +    else
1089 | +        return GetSpecialFolderPath(CSIDL_STARTUP) / "Bitcoin-Qt.lnk";

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

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.

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

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

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

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.

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

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

in src/util.cpp:None in 1cd6d29b9a outdated

 832 |      // Mac: ~/Library/Application Support/Bitcoin
 833 |      // Unix: ~/.bitcoin
 834 |  #ifdef WIN32
 835 |      // Windows
 836 | -    return MyGetSpecialFolderPath(CSIDL_APPDATA, true) / "Bitcoin";
 837 | +    return GetSpecialFolderPath(CSIDL_APPDATA) / "Bitcoin";

sipa commented at 3:51 PM on April 22, 2012:

No looking at APPDATA env variable anymore?

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

You were the one who suggested to remove the fallback, when I added a Win7 compatibility for it :-P.

sipa commented at 3:59 PM on April 22, 2012:

Hmm :)

Diapolo commented at 6:56 AM on April 23, 2012:

I read that as an ACK for that change ;)? If SHGetSpecialFolderPathA() fails there is a serious problem, so fallback should not matter in that case + current version is not Win >= Vista compatible.

sipa commented at 10:40 AM on April 23, 2012:

If the original fallback is unnecessary in windows XP or later (the current requirement for the core code), I'm fine with removing it.

laanwj commented at 11:21 AM on April 23, 2012:

Agreed. I don't think the fallback was ever used. If something as simple as requesting a stock path from the OS fails, there's a serious problem and it's better not to try to guess either...

in src/util.cpp:None in 1cd6d29b9a outdated

 844 | @@ -869,9 +845,12 @@ void PrintExceptionContinue(std::exception* pex, const char* pszThread)
 845 |  
 846 |      LOCK(csPathCached);
 847 |  
 848 | -    if (mapArgs.count("-datadir")) {

sipa commented at 3:53 PM on April 22, 2012:

There was a discussion on IRC recently, and it seemed most agreed to start using a less strict coding style (inspired by the Qt coding style). I suppose we need to change the doc/coding.txt document, though.

Diapolo commented at 6:58 AM on April 23, 2012:

After the last rebase, I reverted to the current used coding style for this.

in src/util.cpp:None in 1cd6d29b9a outdated

1063 | @@ -1092,20 +1064,45 @@ string FormatFullVersion()
1064 |  }
1065 |  
1066 |  #ifdef WIN32
1067 | -boost::filesystem::path static StartupShortcutPath()

sipa commented at 3:54 PM on April 22, 2012:

It was static for a reason: this function was not used outside of the util.cpp compilation unit, no?

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

If it is static I get thrown with compiler warnings.

sipa commented at 4:03 PM on April 22, 2012:

Can I see the code in that case?

Diapolo commented at 5:20 PM on April 22, 2012:

Ahh I got it now, because I added StartupShortcutPath() to util.h and it is static in util.cpp the compiler gives: "declared 'static' but never defined [-Wunused-function]". Going to fix this, sorry.

in src/util.h:None in 1cd6d29b9a outdated

  93 | @@ -94,7 +94,7 @@ T* alignup(T* p)
  94 |  #define _vsnprintf(a,b,c,d) vsnprintf(a,b,c,d)
  95 |  #define strlwr(psz)         to_lower(psz)
  96 |  #define _strlwr(psz)        to_lower(psz)
  97 | -#define MAX_PATH            1024
  98 | +#define MAX_PATH            1024 // Windows: 260 (in windef.h)

sipa commented at 3:55 PM on April 22, 2012:

This is non-WIN32 code, no?

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

I only added it as a hint ... no need to discuss I'll remove it. Gone!

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

1079 | +
1080 | +    printf("SHGetSpecialFolderPathA() failed, could not obtain requested path.\n");
1081 | +    return fs::path("");
1082 | +}
1083 | +
1084 | +boost::filesystem::path static StartupShortcutPath(bool fLegacy = false)

Diapolo commented at 6:56 AM on April 23, 2012:

It's static again, so that is fixed, too.

Diapolo commented at 7:02 AM on April 23, 2012: none

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

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

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

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

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

sipa commented at 4:13 PM on May 3, 2012: member

ACK

Diapolo commented at 9:53 AM on May 5, 2012: none

Rebased, no code changes.

jgarzik commented at 8:24 PM on May 8, 2012: contributor

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 commented at 8:29 PM on May 8, 2012: none

@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 commented at 8:38 PM on May 8, 2012: contributor

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

Diapolo commented at 8:41 PM on May 8, 2012: none

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

laanwj commented at 5:41 AM on May 9, 2012: member

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)

util.h/.ccp: modifiy some comments / rename MyGetSpecialFolderPath() -> 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' 3e468840bd

Diapolo commented at 10:28 AM on May 9, 2012: none

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

laanwj commented at 10:39 AM on May 9, 2012: member

ACK

jgarzik referenced this in commit 11b729d8a2 on May 9, 2012

jgarzik merged this on May 9, 2012

jgarzik closed this on May 9, 2012

coblee referenced this in commit aab68043a5 on Jul 17, 2012

suprnurd referenced this in commit 5128085240 on Dec 5, 2017

lateminer referenced this in commit adfcbf9b71 on Jan 22, 2019

lateminer referenced this in commit a4ded20de4 on Dec 25, 2019

DrahtBot locked this on Sep 8, 2021