Win32: Minimising desktop heap usage

Dave Page <dpage@postgresql.org> writes:

[ get rid of wsprintf in favor of snprintf ]

+1 for not depending on nonstandard subroutines without need.
But please note the standard locution is snprintf(buf, sizeof(buf), ...
Not sizeof() - 1.

! char *tmppath=0;

Please use NULL not 0 ... actually, since the variable is
unconditionally assigned to just below, I'd say this should just be
"char *tmppath;". I don't like useless initializations: if the compiler
is not smart enough to discard them then they waste cycles, and they
also increase the risk of bugs-of-omission in future changes. If
someone were to change things around so that tmppath didn't get assigned
to in one code path, then the compiler would complain about use of an
uninitialized variable --- *unless* you've written a useless
initialization such as the above, in which case the mistake might pass
unnoticed for awhile. So don't initialize a local variable unless
you're giving it an actually meaningful value.

! /*
! * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop heap.
! */
! tmppath = getenv("APPDATA");

Hmm, is there any functional downside to this? I suppose
SHGetSpecialFolderPath does some things that getenv does not...

Otherwise it looks good...

regards, tom lane

Tom Lane wrote:

Dave Page <dpage@postgresql.org> writes:

[ get rid of wsprintf in favor of snprintf ]

+1 for not depending on nonstandard subroutines without need.
But please note the standard locution is snprintf(buf, sizeof(buf), ...
Not sizeof() - 1.

Noted.

! char *tmppath=0;

Please use NULL not 0 ... actually, since the variable is
unconditionally assigned to just below, I'd say this should just be
"char *tmppath;". I don't like useless initializations: if the compiler
is not smart enough to discard them then they waste cycles, and they
also increase the risk of bugs-of-omission in future changes. If
someone were to change things around so that tmppath didn't get assigned
to in one code path, then the compiler would complain about use of an
uninitialized variable --- *unless* you've written a useless
initialization such as the above, in which case the mistake might pass
unnoticed for awhile. So don't initialize a local variable unless
you're giving it an actually meaningful value.

The downside is that we see a useless warning that, if sufficiently
multiplied, might make it hard to see something we are interested in.

In this case, not initialising it will also create the first 'accepted'
warning in VC++ in our code :-(. All the others come from Kerberos.

Modified in the attached patch however.

! /*
! * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop heap.
! */
! tmppath = getenv("APPDATA");

Hmm, is there any functional downside to this? I suppose
SHGetSpecialFolderPath does some things that getenv does not...

A good percentage of the special folder paths you might query with
SHGetSpecialFolderPath() are not actually in the environment. APPDATA
certainly is on XP, 2K3 and Vista though, and I've found MS KB articles
referring to it being on 2K as well.

Regards, Dave.

Dave Page <dpage@postgresql.org> writes:

Tom Lane wrote:

So don't initialize a local variable unless
you're giving it an actually meaningful value.

The downside is that we see a useless warning that, if sufficiently
multiplied, might make it hard to see something we are interested in.

[ looks again... ] Actually, I think you just proved my point for me.
The ZeroMemory call should go away, no? (Does this mean that the
Windows builds fail to detect dereferencing NULL? Bad if so.)

Hmm, is there any functional downside to this? I suppose
SHGetSpecialFolderPath does some things that getenv does not...

A good percentage of the special folder paths you might query with
SHGetSpecialFolderPath() are not actually in the environment. APPDATA
certainly is on XP, 2K3 and Vista though, and I've found MS KB articles
referring to it being on 2K as well.

OK, in that case seems no problem.

regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

! /*
! * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop heap.
! */
! tmppath = getenv("APPDATA");

Hmm, is there any functional downside to this? I suppose
SHGetSpecialFolderPath does some things that getenv does not...

The functional difference appears to be that the environment variable is set
on startup (or login?) and doesn't necessarily have the most up to date value
if it's been changed. But it's not something you're likely to change and you
can always adjust the environment variable manually to fix the problem.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Tom Lane wrote:

[ looks again... ] Actually, I think you just proved my point for me.
The ZeroMemory call should go away, no?

Yup, quite correct. v3 attached.

/D

Gregory Stark wrote:

"Tom Lane" <tgl@sss.pgh.pa.us> writes:

! /*
! * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
! * will force us to link with shell32.lib which eats valuable desktop heap.
! */
! tmppath = getenv("APPDATA");

Hmm, is there any functional downside to this? I suppose
SHGetSpecialFolderPath does some things that getenv does not...

The functional difference appears to be that the environment variable is set
on startup (or login?) and doesn't necessarily have the most up to date value
if it's been changed. But it's not something you're likely to change and you
can always adjust the environment variable manually to fix the problem.

If you're hacking the registry to change it then you've only got
yourself to blame if you don't update the environment as well imho.

I also wouldn't be at all surprised if many apps build paths containing
the value returned by SHGetSpecialFolderPath() (or %APPDATA%) at startup
and then use that value from then on rather than repeatedly calling the
API function. It's not like you'd expect the value to change at all
often, if ever.

/D

Tom Lane wrote:

Dave Page <dpage@postgresql.org> writes:

Tom Lane wrote:

So don't initialize a local variable unless
you're giving it an actually meaningful value.

The downside is that we see a useless warning that, if sufficiently
multiplied, might make it hard to see something we are interested in.

[ looks again... ] Actually, I think you just proved my point for me.
The ZeroMemory call should go away, no? (Does this mean that the
Windows builds fail to detect dereferencing NULL? Bad if so.)

Windows builds don't fail to detect that in genereal. ZeroMemory,
however, has a protection specifically against being passed NULL input,
IIRC.

//Magnus

Dave Page wrote:

Tom Lane wrote:

[ looks again... ] Actually, I think you just proved my point for me.
The ZeroMemory call should go away, no?

Yup, quite correct. v3 attached.

Great job tracking this down!

Patch looks good from here (after the fixes per Tom).

Applied, many thanks!

//Magnus