Updates / cleanup for utility functions #1066

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.

This one broke MAX_PATH limits, which seems bad.

How often does the code call real_strprintf()? This change means clearing 50K or memory every time it is called, which might be a performance issue if it is called a lot.

I can check, but I think it's good style to init used arrays or variables, as this creates a clean environment for code relying on that array. What's your oppinion here?

Please check. It is absolutely good style, but if it make the GUI super sluggish because we're calling it 100 times per second when computing the list of transactions to show then performance trumps style (I have no idea if we are or not, but don't want people to start complaining that the 0.6.1 release is a lot slower than 0.6....)

real_strprintf is called a lot of times in many places (for example, HexStr).

I don't think clearing it every time adds anything. The buffer is only written by _vsnprintf, which has well-defined semantics and doesn't do anything with the contents of the buffer beforehand. It returns the number of characters that will be in the output string. Nothing in the strprintf function relies on zero bytes anywhere.

If real_strprintf is "safe" and you are right, it's quite often called ... I'm fine with removing the init. In general my intention was to reduce the risk of buffer-overflows / buffer-corruptions causing weird things to happen. It would be nice, if we were able to rewrite funtcions to use std::string instead of char * or char arrays.

It would be nice, if we were able to rewrite funtcions to use std::string instead of char * or char arrays

We certainly agree on that. Apart from places where you really can't get around it such as strprintf, which wraps a legacy C function, there should be as few fixed-size buffers used as possible.

Removed the memset() in the last rebase!

Why rename this variable, buffer is a fine name?

I suppose he's following the code guidelines - you know, the contents of doc/coding.txt? ;)

Variable names without a prefix break coding style standard I think ... somethimes I really would like to have mandantory rules, that I can use while coding. I don't insist on that rename, but if you use buffer, you simply don't know what type it is.

Can't say I really like this guideline. Encoding types into variable names reeks of the windows "Hungarian notation" fad, and makes little sense to me. IMO, variable names should be clear and human readable. My IDE and compiler figure out the rest from the context.

Then again, if you think this makes the util code more consistent feel free to do it here. Please don't do it throughout the qt code though.

I don't think many people except for Satoshi like this coding standard, actually, but I must say I like the consistency.

That said, I never realized it wasn't used in the Qt code...

Eventually it makes no sense in C++ anyway, as there are so many types and classes.

@laanwj I don't need psz or f or n in front of variables, but at least the names should speak and be consistent everywhere.

Reverted to buffer and will keep using this ^^.

Why "pszBuffer"?

Here it's buffer again, too ;).

Shouldn't this probably be Bitcoin-Qt.lnk anyway?

That's a nice suggestion, perhaps delete the old one if it exists and create a new Bitcoin-Qt.lnk.

Does the name of the file matter here? It'll just be overwritten with a correct link right now. I don't think there a pressing need to add rename+delete logic.

This introduces a bug. Uninitialized char arrays are not guaranteed to be null.

Take a look into ParseParameters() init happens there via memset.

It's only memset if -datadir is specified...

Alright, that can be changed and thanks :-D.

I re-added this ;).

See my other remark. Please keep it at ="" or ={0} (equivalent), which does the same in ~5% of the characters.

Is there a need to initialize pszFile/etc to null?

Good coding style IMHO.

Bad coding style, IMHO.

I am very sure (but I don't have time to check with the C standard right now) that ="" is the same as ={0}; and does exactly the same as memset: initialize the array to zeros. No security theater please, I'd like to keep it at "" which is clearest to developers.

@laanwj You convinced me, the important part is not via which method an array is initialized as long as we just initialize it :).

from http://www.cplusplus.com/doc/tutorial/arrays/ "When declaring a regular array of local scope (within a function, for example), if we do not specify otherwise, its elements will not be initialized to any value by default, so their content will be undetermined until we store some value in them. The elements of global and static arrays, on the other hand, are automatically initialized with their default values, which for all fundamental types this means they are filled with zeros."

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.

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.

Is pszBuffer still used now?

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