Updates / cleanup for utility functions #1066

Diapolo · 2012-04-08T22:40:11Z

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".

sipa · 2012-04-08T22:58:09Z

src/util.cpp

    {
-        return getenv("APPDATA");
+        // Windows < Vista: C:\Documents and Settings\Username\StartMenu\Programs\Startup


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.

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.

I'd prefer simply giving back an error, instead of guessing and fooling around. I suppose if GetSpecialFolderPath fails something is wrong anyway.

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.

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.

sipa · 2012-04-09T22:07:16Z

I've done some similar cleanups (only path-handling related) in #1072.

Diapolo · 2012-04-10T07:08:39Z

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.

laanwj · 2012-04-10T07:13:03Z

src/util.cpp

            va_list arg_ptr;
            va_start(arg_ptr, pszFormat);
-            int limit = END(pszBuffer) - pend - 2;
+            int limit = END(staticBuffer) - pend - 2;
            int ret = _vsnprintf(pend, limit, pszFormat, arg_ptr);
            va_end(arg_ptr);
            if (ret < 0 || ret >= limit)
            {


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

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.

laanwj · 2012-04-10T07:13:21Z

Thanks, looking good now.

luke-jr · 2012-04-10T13:54:36Z

Rebasing required.

luke-jr · 2012-04-10T16:58:11Z

src/util.cpp

-    pszModule[0] = '\0';
-    GetModuleFileNameA(NULL, pszModule, sizeof(pszModule));
+    char module[MAX_PATH] = "";
+    GetModuleFileNameA(NULL, module, sizeof(module));
 #else
    const char* pszModule = "bitcoin";


If you're going to rename pszModule, you need to do it here too...

Fixed, that went through because I used a Refactoring function, which didn't catch this ;). Thank you for watching.

Diapolo · 2012-04-10T17:47:12Z

As I said I would like to get in #1072 before, as I will have to rebase against that :).

sipa · 2012-04-10T18:03:44Z

You can already rebase against #1072, if you like, but you risk needing to change things if #1072 changes before being merged then.

sipa · 2012-04-11T22:13:45Z

I merged #1072.

Diapolo · 2012-04-12T04:59:19Z

Nice, will rebase later today :).

Diapolo · 2012-04-12T06:57:47Z

Rebased and I'm looking through the code again to verify...

sipa · 2012-04-12T14:24:50Z

src/util.cpp

@@ -196,23 +196,24 @@ inline int OutputDebugStringF(const char* pszFormat, ...)
        {
            LOCK(cs_OutputDebugStringF);
            static char pszBuffer[50000];


Is pszBuffer still used now?

That must be a merge misstake I overlooked ... removed :), thanks for your hint!

…legacy shortcut gets updated

Diapolo · 2012-04-12T16:45:35Z

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.

luke-jr · 2012-04-12T21:10:38Z

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.

Diapolo · 2012-04-12T21:28:48Z

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).

sipa · 2012-04-12T22:56:53Z

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.

gavinandresen · 2012-04-12T23:19:25Z

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:

Fix a real bug (we have 180 of them, there are plenty to choose from), and do a little code cleanup as part of the fix.
Add some unit tests, and clean up the code you are unit-testing (make sure you run the unit tests both before and after the cleanup).
Add a new feature, and clean up any code that has to be changed to support the new feature.

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.

freewil · 2012-04-13T00:12:47Z

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"

laanwj · 2012-04-13T05:58:33Z

I like bold code cleanups, such as code de-duplication, eliminating unneeded functions, replacing archaic and dangerous C with modern C++, grouping together similar functionality, isolating different concerns, untangling knots of unrelated include files, and so on.

Oh yes and: fixing compiler warnings! Those boatloads of warnings are a shame to the project.

On the other hand changing namespace references, or renaming local variables, as you see it causes a lot of bikeshedding discussion for no real good. yes, everyone has their own feeling about such things. It's better to harmonize people than harmonize code in this case, and just leave it be...

Combining bug fixes with a little related cleanup (as you did with the QR patch) is great though. It's nice to not be the only person that works on the UI.

Diapolo · 2012-04-13T09:46:27Z

Allright, 2 options for me ... leave this as it is or rework to change no var names and only add the small changes I did code-wise. First vore wins, let's go :D.

Edit: @laanwj Thanks, I'm more the GUI lover than console user and it's fun to work together :).

gavinandresen · 2012-04-13T23:57:37Z

First vote: rework.

Diapolo · 2012-04-21T13:13:12Z

I'm back and will do this in the next days :) and btw. I like clear orders ^^.

* fix forking logic so that mining generate size is just set once at the moment of the fork, while keeping the enforced minimum on the EB and datacarrier sizes

0cf9bb0 [Cleanup][Refactor][Performance] Main.cpp code cleanup: * Not used GetInputAgeIX method removed. * AcceptToMemoryPool HasZerocoinSpendInputs code duplication. * Several for loops copying the variable cleaned. (furszy) Pull request description: * Not used GetInputAgeIX method removed. * AcceptToMemoryPool HasZerocoinSpendInputs code duplication. * Several for loops copying variable cleaned. ACKs for top commit: random-zebra: ACK 0cf9bb0 Warrows: ACK 0cf9bb0 Tree-SHA512: fc443266b34e3730ad5d9ed2d698ef101cef4d8565b8070a0da6df9139201c4af5b1f6e992b24f3e8a009c8d56df14e1d14b3a620a05fa17d5ac3f08fa12e3d3

4d7dd0c Add release notes for Omni Core 0.7.1 (dexX7) 92cf8e9 Bump version to 0.7.1 and update tests (dexX7) Pull request description: This pull request updates the version of Omni Core 0.7.1 and adds tests for it. It also updates the release notes of this version. Tree-SHA512: d10edb3ee872caca7a3c307955677ef213e2bd92e22c0a3eae3978c0b1b910f24c0e8e2b2ca9931e6de40aa2434db8e3d99380a926887782bd85f44a04e18d79

sipa reviewed Apr 8, 2012
View reviewed changes

laanwj reviewed Apr 10, 2012
View reviewed changes

luke-jr reviewed Apr 10, 2012
View reviewed changes

Diapolo mentioned this pull request Apr 10, 2012

Use filesystem::path instead of manual string tinkering #1072

Merged

sipa reviewed Apr 12, 2012
View reviewed changes

Philip Kaufmann added 3 commits April 12, 2012 18:16

Updates / cleanup for utility functions

2f1f654

rename Windows Autostart shortcut to "Bitcoin-Qt.lnk" and ensure the …

dc21c29

…legacy shortcut gets updated

removed an unused var that I overlooked while resolving rebase conflicts

081ba7f

gavinandresen closed this Apr 13, 2012

suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017

new PoSe - MNVERIFY (bitcoin#1066)

342bda5

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Updates / cleanup for utility functions #1066

Updates / cleanup for utility functions #1066

Uh oh!

Conversation

Diapolo commented Apr 8, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Apr 9, 2012

Uh oh!

Diapolo commented Apr 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 10, 2012

Uh oh!

luke-jr commented Apr 10, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Apr 10, 2012

Uh oh!

sipa commented Apr 10, 2012

Uh oh!

sipa commented Apr 11, 2012

Uh oh!

Diapolo commented Apr 12, 2012

Uh oh!

Diapolo commented Apr 12, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Apr 12, 2012

Uh oh!

luke-jr commented Apr 12, 2012

Uh oh!

Diapolo commented Apr 12, 2012

Uh oh!

sipa commented Apr 12, 2012

Uh oh!

gavinandresen commented Apr 12, 2012

Uh oh!

freewil commented Apr 13, 2012

Uh oh!

laanwj commented Apr 13, 2012

Uh oh!

Diapolo commented Apr 13, 2012

Uh oh!

gavinandresen commented Apr 13, 2012

Uh oh!

Diapolo commented Apr 21, 2012

Uh oh!

Uh oh!