Updates / cleanup for utility functions#1066
Updates / cleanup for utility functions#1066Diapolo wants to merge 3 commits intobitcoin:masterfrom Diapolo:util-updates
Conversation
src/util.cpp
Outdated
There was a problem hiding this comment.
Is there no way of asking windows for the startup folder, in a version-agnostic way? Seems hard to believe one would need guesswork like this.
There was a problem hiding this comment.
If SHGetSpecialFolderPathA() fails we HAVE to guess (our fallback) or GetSpecialFolderPath() fails ... there is NO environment variable that points to the autostart folder and MS changed the folder in Vista and higher in comparison to i.e. WinXP.
There was a problem hiding this comment.
I'd prefer simply giving back an error, instead of guessing and fooling around. I suppose if GetSpecialFolderPath fails something is wrong anyway.
There was a problem hiding this comment.
The fallback WAS in before I edited the file, but it would have failed for Vista and higher. If you are fine with the rest of the changes I can remove this, sure.
There was a problem hiding this comment.
I think it's fine to change it as you did. But I'd also be fine with removing the fallback and returning error.
There are places where extensive recovery mechanisms for OS errors are warranted, but this isn't one of them (how likely is requesting a string from the OS to fail?). No need to spend much time on this.
|
I've done some similar cleanups (only path-handling related) in #1072. |
|
Last commit reverts to old style of array init via = "", which is allowed and shorter than a memset(). At least now all arrays are initialized :). The only exception is in real_strprintf() where the init could perhaps slow down the GUI / client. |
There was a problem hiding this comment.
I wonder if we couldn't reduce all this wonderfully verbose code to about two lines of C++ string manipulation? :-) (or even better, reuse strprintf? At least by adding an intermediate function vstrprintf that takes a va_arg list)
Edit: doesn't necessarily need to be in this patch though, just a comment in general
There was a problem hiding this comment.
Every simplyfication is a great step, so I agree ... but I don't want to bloat this pull request :). I would like sipas boost pathm rework merged into master before my util updates get in, as he has a better GetDataDir() version.
|
Thanks, looking good now. |
|
Rebasing required. |
src/util.cpp
Outdated
There was a problem hiding this comment.
If you're going to rename pszModule, you need to do it here too...
There was a problem hiding this comment.
Fixed, that went through because I used a Refactoring function, which didn't catch this ;). Thank you for watching.
|
As I said I would like to get in #1072 before, as I will have to rebase against that :). |
|
I merged #1072. |
|
Nice, will rebase later today :). |
|
Rebased and I'm looking through the code again to verify... |
src/util.cpp
Outdated
There was a problem hiding this comment.
That must be a merge misstake I overlooked ... removed :), thanks for your hint!
|
Rebased one more ;-). I'm only going to fix glitches / errors that are currently in my changes, as I don't want to bloat this. There is of course plenty of room for optimizing our utility functions, but that should go to seperate pull requests. |
|
Most of this looks like it makes the code more unreadable. IMO, the absolute namespaces are far clearer, and should be encouraged, while the general "using namespace" discouraged. sipa's per-function "using namespace foo = boost::longerfoo" makes sense and doesn't interfere with readability. |
|
God damn ... everytime I try to harmonize the code someone has an own feeling about things. BUT most of the time I see things in the code + in many places and use these as base. I have no problem to completely discard the global use of "using namespace" if everyone would do it. Function local using namespaces seem much more weird to me. But even if you don't like the style ... there are at least valuable fixes in here, like init buffers that were not before, renaming Bitcoin.lnk to Bitcoin-Qt.lnk and the removal of a fallback for a Windows function that sipa suggested. The whole work on filesystem::path was obsolete as Sipa did that (and yes even better then I could). |
|
I really don't care about namespaces, and only followed the pattern of the code already there when doing the boost::filesystem::path stuff. As long as there are no namespaces in header files, I don't care. |
|
56 comments for such trivial changes... I think there are higher priority things we could be spending our time on. This is why I don't like "I'll just clean up the code because I can" changes. I would much prefer to see code cleaned up as it is being improved, so:
Diapolo: we have to weigh the benefits of "cleaner" code against the potential costs of ANY changes to the code. It is incredibly easy to screw up and let one tiny little change through that has a really bad unintended consequence. |
|
May I suggest for code cleanup pull requests, you make a smaller and simpler pull request for each one such as "make sure all char arrays in util.cpp get initialized to 0 and use sizeof(CharArray) instead of MAX_LEN" |
6 participants
Removed GetSpecialFolderPath() fallbacks (as requested), removed several unneeded spaces, renamed MyGetSpecialFolderPath() -> GetSpecialFolderPath() as the first one sounds not very pro ^^, make sure all char arrays in util.cpp get initialized to 0 and use sizeof(CharArray) instead of MAX_LEN, renamed Windows Autostart shortcut to "Bitcoin-Qt.lnk".