Code review - Issue 12665047: Fixes compilation with Visual Studio 2013.https://codereview.appspot.com/2013-09-17T19:38:40+00:00rietveld
Message from unknown
2013-08-10T18:59:25+00:00bfhbenurn:md5:cb74dde05e31d7e74c48ade8b0aef01b
Message from bfhben@gmail.com
2013-08-25T21:47:39+00:00bfhbenurn:md5:331e434ca72f5d39b7db504ac857de6b
Message from shannonwoods@chromium.org
2013-08-26T16:49:37+00:00Shannon Woodsurn:md5:1ea84bc1837f7d9dbd4c16b9c97b3254
Support for an unreleased version of Visual Studio isn't super-high priority for ANGLE at the moment, so I'd like to keep any changes we accept to that end as minimal as possible.
https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h
File src/libGLESv2/renderer/Renderer.h (left):
https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#oldcode21
src/libGLESv2/renderer/Renderer.h:21: const int versionWindows7 = MAKEWORD(0x01, 0x06);
Why has the Windows 7 version const been removed?
https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h
File src/libGLESv2/renderer/Renderer.h (right):
https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#newcode26
src/libGLESv2/renderer/Renderer.h:26: int minor = LOBYTE(version);
This has the major & minor version bytes swapped versus the code replaced.
https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#newcode38
src/libGLESv2/renderer/Renderer.h:38: return VerifyVersionInfo(&osVersionInfo, VER_MINORVERSION | VER_MAJORVERSION, conditionMask) == TRUE;
GetVersionEx looks like it would probably be closer to what the original code did, and would let us use direct comparisons instead of macros and masks.
Message from bfhben@gmail.com
2013-08-26T16:53:54+00:00bfhbenurn:md5:8fae0973dfb538b0c4093247f7f73c63
On 2013/08/26 16:49:37, shannonwoods wrote:
> Support for an unreleased version of Visual Studio isn't super-high priority for
> ANGLE at the moment, so I'd like to keep any changes we accept to that end as
> minimal as possible.
>
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h
> File src/libGLESv2/renderer/Renderer.h (left):
>
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#oldcode21
> src/libGLESv2/renderer/Renderer.h:21: const int versionWindows7 = MAKEWORD(0x01,
> 0x06);
> Why has the Windows 7 version const been removed?
>
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h
> File src/libGLESv2/renderer/Renderer.h (right):
>
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#newcode26
> src/libGLESv2/renderer/Renderer.h:26: int minor = LOBYTE(version);
> This has the major & minor version bytes swapped versus the code replaced.
>
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#newcode38
> src/libGLESv2/renderer/Renderer.h:38: return VerifyVersionInfo(&osVersionInfo,
> VER_MINORVERSION | VER_MAJORVERSION, conditionMask) == TRUE;
> GetVersionEx looks like it would probably be closer to what the original code
> did, and would let us use direct comparisons instead of macros and masks.
The Windows 7 version const has been removed due to the fact it was unused, it could always be readded. VerifyVersionInfo is used since GetVersionEx is also deprecated, and the use in this case in Microsoft's terminology is "you must be this high" testing, which VerifyVersionInfo is designed for. I do not believe I had the bytes swapped, I did test this in an isolated case with different version numbers and appeared to get the desired results.
Message from bfhben@gmail.com
2013-08-26T17:21:26+00:00bfhbenurn:md5:8d8ea0ee9053e4916b7bd877cd0a7567
On 2013/08/26 16:53:54, bfhben wrote:
> On 2013/08/26 16:49:37, shannonwoods wrote:
> > Support for an unreleased version of Visual Studio isn't super-high priority
> for
> > ANGLE at the moment, so I'd like to keep any changes we accept to that end as
> > minimal as possible.
> >
> >
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h
> > File src/libGLESv2/renderer/Renderer.h (left):
> >
> >
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#oldcode21
> > src/libGLESv2/renderer/Renderer.h:21: const int versionWindows7 =
> MAKEWORD(0x01,
> > 0x06);
> > Why has the Windows 7 version const been removed?
> >
> >
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h
> > File src/libGLESv2/renderer/Renderer.h (right):
> >
> >
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#newcode26
> > src/libGLESv2/renderer/Renderer.h:26: int minor = LOBYTE(version);
> > This has the major & minor version bytes swapped versus the code replaced.
> >
> >
> https://codereview.appspot.com/12665047/diff/1/src/libGLESv2/renderer/Renderer.h#newcode38
> > src/libGLESv2/renderer/Renderer.h:38: return VerifyVersionInfo(&osVersionInfo,
> > VER_MINORVERSION | VER_MAJORVERSION, conditionMask) == TRUE;
> > GetVersionEx looks like it would probably be closer to what the original code
> > did, and would let us use direct comparisons instead of macros and masks.
>
> The Windows 7 version const has been removed due to the fact it was unused, it
> could always be readded. VerifyVersionInfo is used since GetVersionEx is also
> deprecated, and the use in this case in Microsoft's terminology is "you must be
> this high" testing, which VerifyVersionInfo is designed for. I do not believe I
> had the bytes swapped, I did test this in an isolated case with different
> version numbers and appeared to get the desired results.
Ah sorry, I noticed what you meant with regards to major and minor being swapped, that's because I'm now using the version argument and getting the low and high bytes from that, not the low and high bytes of GetVersion. The macro MAKEWORD does it in that order from what I can tell.
Message from scottmg@chromium.org
2013-09-17T19:24:16+00:00scottmgurn:md5:c3b5ddf30dc56237a7345221a8944a4a
Any chance we could land either this or this simpler patch https://codereview.appspot.com/13550046/ to get compilation working?
Message from shannonwoods@chromium.org
2013-09-17T19:38:40+00:00Shannon Woodsurn:md5:d4e39557d602b8cd303a2828ca70ce5e
On 2013/09/17 19:24:16, scottmg wrote:
> Any chance we could land either this or this simpler patch
> https://codereview.appspot.com/13550046/ to get compilation working?
The Windows 7 version still needs to be readded in this patch; the other one can move forward in the meantime.