Re: [1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Juan Lang]

Hi Adam,
perhaps I'm just being obtuse, but I don't see how Dmitry's comment
has been addressed for this patch.  I'm confused by the commit comment
("Emulate Win9x if appropriate") and the change itself:

     /* Win98 returns only the swapsize in ullTotalPageFile/ullAvailPageFile,
        WinXP returns the size of physical memory + swapsize;
-       mimic the behavior of XP.

You are removing the part of the comment explaining what the code
actually does, which I find confusing.  If you're part of the "the
code is the documentation" crowd, just remove the entire comment.  If
not, please add a brief explanation.

        Note: Project2k refuses to start if it sees less than 1Mb of free swap.
     */
-    lpmemex->ullTotalPageFile += lpmemex->ullTotalPhys;
-    lpmemex->ullAvailPageFile += lpmemex->ullAvailPhys;
-
-    /* Titan Quest refuses to run if TotalPageFile <= ullTotalPhys */
-    if(lpmemex->ullTotalPageFile == lpmemex->ullTotalPhys)
+    if (osver.dwMajorVersion >= 5 || osver.dwPlatformId !=
VER_PLATFORM_WIN32_WINDOWS)

This change is confusing to me.  dwPlatformId == VER_PLATFORM_WIN32_NT
for NT4, Win2k, XP, etc.  dwMajorVersion is never greater than 4 on
Windows 9x.  In other words, do you mean to exclude NT4 here, or not?
If so, then the first check should suffice.  If not, then the second
check should.  But in either case, what does this have to do with
being "appropriate" for Win9x emulation?
--Juan

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Adam Martinson]

On 06/06/2011 10:23 AM, Juan Lang wrote:
> Hi Adam,
> perhaps I'm just being obtuse, but I don't see how Dmitry's comment
> has been addressed for this patch.  I'm confused by the commit comment
> ("Emulate Win9x if appropriate") and the change itself:
>
> /* Win98 returns only the swapsize in ullTotalPageFile/ullAvailPageFile,
>          WinXP returns the size of physical memory + swapsize;
> -       mimic the behavior of XP.
>
> You are removing the part of the comment explaining what the code
> actually does, which I find confusing.  If you're part of the "the
> code is the documentation" crowd, just remove the entire comment.  If
> not, please add a brief explanation.

I'm removing part of the comment because it no longer applies.  We're 
not just using the XP behavior for everything anymore, now we're using 
the 9x behavior when it's appropriate.
> Note: Project2k refuses to start if it sees less than 1Mb of free swap.
>       */
> -    lpmemex->ullTotalPageFile += lpmemex->ullTotalPhys;
> -    lpmemex->ullAvailPageFile += lpmemex->ullAvailPhys;
> -
> -    /* Titan Quest refuses to run if TotalPageFile<= ullTotalPhys */
> -    if(lpmemex->ullTotalPageFile == lpmemex->ullTotalPhys)
> +    if (osver.dwMajorVersion>= 5 || osver.dwPlatformId !=
> VER_PLATFORM_WIN32_WINDOWS)
>
> This change is confusing to me.  dwPlatformId == VER_PLATFORM_WIN32_NT
> for NT4, Win2k, XP, etc.  dwMajorVersion is never greater than 4 on
> Windows 9x.  In other words, do you mean to exclude NT4 here, or not?
> If so, then the first check should suffice.  If not, then the second
> check should.  But in either case, what does this have to do with
> being "appropriate" for Win9x emulation?
> --Juan

if (osver.dwMajorVersion >= 5 || osver.dwPlatformId  == 
VER_PLATFORM_WIN32_NT)
would be equivalent.  The stuff in that block is only for NT/2k+.  In 9x 
more we *don't* do that.

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Juan Lang]

> I'm removing part of the comment because it no longer applies.  We're not
> just using the XP behavior for everything anymore, now we're using the 9x
> behavior when it's appropriate.

When is it appropriate, and when not?  I don't know, and your patch
doesn't explain that.  See below for more on why it's confusing.
> if (osver.dwMajorVersion >= 5 || osver.dwPlatformId  ==
> VER_PLATFORM_WIN32_NT)
> would be equivalent.  The stuff in that block is only for NT/2k+.  In 9x
> more we *don't* do that.

Let me say it again:  the condition you've written doesn't return TRUE
when running NT4 mode.  Why is that, and what's that got to do with
Win9x?  If this wasn't intended, then your check if wrong.  If it was,
then the commit message is wrong.  In either case, something is wrong
with your patch as it is.
--Juan

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Adam Martinson]

On 06/06/2011 10:45 AM, Juan Lang wrote:
> > I'm removing part of the comment because it no longer applies.  We're not
> > just using the XP behavior for everything anymore, now we're using the 9x
> > behavior when it's appropriate.
> > When is it appropriate, and when not?  I don't know, and your patch
> > doesn't explain that.  See below for more on why it's confusing.
> > if (osver.dwMajorVersion>= 5 || osver.dwPlatformId  ==
> > VER_PLATFORM_WIN32_NT)
> > would be equivalent.  The stuff in that block is only for NT/2k+.  In 9x
> > more we *don't* do that.
> > Let me say it again:  the condition you've written doesn't return TRUE
> > when running NT4 mode.  Why is that, and what's that got to do with
> > Win9x?  If this wasn't intended, then your check if wrong.  If it was,
> > then the commit message is wrong.  In either case, something is wrong
> > with your patch as it is.
> > --Juan
> > In NT4 mode:
> > (osver.dwMajorVersion>= 5) is FALSE.
> > (osver.dwPlatformId != VER_PLATFORM_WIN32_WINDOWS) is TRUE.
> > (osver.dwMajorVersion>= 5 || osver.dwPlatformId != 
> > VER_PLATFORM_WIN32_WINDOWS) is TRUE.
> > Right?

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Juan Lang]

> In NT4 mode:
> (osver.dwMajorVersion>= 5) is FALSE.
> (osver.dwPlatformId != VER_PLATFORM_WIN32_WINDOWS) is TRUE.
> (osver.dwMajorVersion>= 5 || osver.dwPlatformId !=
> VER_PLATFORM_WIN32_WINDOWS) is TRUE.
> Right?

Ah, right.  Wrong with this comment, sorry about that.  But my
original comment remains:  why not just check dwPlatformId?
--Juan

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Adam Martinson]

On 06/06/2011 10:50 AM, Juan Lang wrote:
> > In NT4 mode:
> > (osver.dwMajorVersion>= 5) is FALSE.
> > (osver.dwPlatformId != VER_PLATFORM_WIN32_WINDOWS) is TRUE.
> > (osver.dwMajorVersion>= 5 || osver.dwPlatformId !=
> > VER_PLATFORM_WIN32_WINDOWS) is TRUE.
> > Right?
> > Ah, right.  Wrong with this comment, sorry about that.  But my
> > original comment remains:  why not just check dwPlatformId?
> > --Juan
> > Ha!  Quite right, thanks!

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Juan Lang]

> Ha!  Quite right, thanks!

If that's what you want, then an easier check is to use GetVersion()
rather than GetVersionEx.  (GetVersion() & 0x80000000) is true under
Win9x, false under any NT version.  See many checks like this in e.g.
kernel32.
--Juan

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Adam Martinson]

On 06/06/2011 11:02 AM, Juan Lang wrote:
> > Ha!  Quite right, thanks!
> > If that's what you want, then an easier check is to use GetVersion()
> > rather than GetVersionEx.  (GetVersion()&  0x80000000) is true under
> > Win9x, false under any NT version.  See many checks like this in e.g.
> > kernel32.
> > --Juan
> > GetVersionEx() is used in GlobalMemoryStatus() also, after these are in 
> > I'll pull it out and make it static, and then just do it once and use it 
> > for both.

[1/3] kernel32/heap: Emulate Win9x if appropriate in GlobalMemoryStatusEx(). (try 2)

[Juan Lang]

> GetVersionEx() is used in GlobalMemoryStatus() also, after these are in I'll
> pull it out and make it static, and then just do it once and use it for
> both.

Okay, thanks.

Unrelated nit on patch 3/3:
+    lpmemex->ullTotalVirtual  =
min((ULONG_PTR)si.lpMaximumApplicationAddress-(ULONG_PTR)si.lpMinimumApplicationAddress,
ullTotalVirtualMax);
     /* FIXME: we should track down all the already allocated VM pages
and subtract them, for now arbitrarily remove 64KB so that it matches
NT */

This line is too long, please place the comment on a line by itself.

Thanks,
--Juan