Fixed invalid cast on mingw.

Fixed invalid cast on mingw.

Fixes GCC error:
SkThread_win.cpp:39:61: error: invalid static_cast from type ‘int32_t {aka int}’ to type ‘const volatile LONG& {aka const volatile long int&}’

Committed: https://code.google.com/p/skia/source/detail?r=9706

[caryclark, 2013-06-18 11:57:55]

When I try this change in a msvn build, I get

2>d:\cygwin\skgit\trunk\src\ports\skthread_win.cpp(39): error C2220: warning
treated as error - no 'object' file generated
2>d:\cygwin\skgit\trunk\src\ports\skthread_win.cpp(39): warning C4197: 'volatile
const LONG' : top-level volatile in cast is ignored

You can repro this by using a trybot. Click on the "try more bots" link next to
the commit button.

https://codereview.chromium.org/17047004/diff/1/src/ports/SkThread_win.cpp
File src/ports/SkThread_win.cpp (right):

https://codereview.chromium.org/17047004/diff/1/src/ports/SkThread_win.cpp#ne...
src/ports/SkThread_win.cpp:39: LONG value = static_cast<LONG const
volatile>(*addr);
is the const still required?

[cjacek, 2013-06-19 14:08:12]

Thanks for quick review!

I couldn't figure out how to use 'Try more bots', maybe I don't have enough
privs. Anyway, looking back at the patch, it seems like a cleaner solution is
declaring the whole value variable as volatile. I uploaded an updated patch.

[caryclark, 2013-06-19 14:23:07]

https://codereview.chromium.org/17047004/diff/5001/src/ports/SkThread_win.cpp
File src/ports/SkThread_win.cpp (right):

https://codereview.chromium.org/17047004/diff/5001/src/ports/SkThread_win.cpp...
src/ports/SkThread_win.cpp:39: volatile LONG value = *addr;
This change isn't equivalent. This makes the L-value volatile, but we want the
R-value to be volatile.

[scroggo, 2013-06-19 14:36:34]

Adding Ben, who originally wrote this function.

[cjacek, 2013-06-20 11:02:47]

On 2013/06/19 14:23:07, caryclark wrote:
> This change isn't equivalent. This makes the L-value volatile, but we want the
> R-value to be volatile.

Yes, I know, but it should be fine. As far as I understand, the intention of
using volatile is kept in this patch. I attached a new patch that casts R-value,
but treats it as value instead of reference.

[bungeman-skia, 2013-06-20 14:30:59]

On 2013/06/20 11:02:47, cjacek wrote:
> On 2013/06/19 14:23:07, caryclark wrote:
> > This change isn't equivalent. This makes the L-value volatile, but we want
the
> > R-value to be volatile.
> 
> Yes, I know, but it should be fine. As far as I understand, the intention of
> using volatile is kept in this patch. I attached a new patch that casts
R-value,
> but treats it as value instead of reference.

The purpose of this cast is the 'classical' use of volatile, to prevent the
compiler from putting '*addr' into a register (force a load instruction) or
optimizing the loop into an infinite do-nothing loop after the first failure. No
extra VC++ implied memory barriers are wanted or needed (on x86 and x86_64
they're no-ops anyway). The reason this is written this way is, frankly, because
that's how boost did it.

It may be possible to write this more like the implementation in
SkThread_pthread.cpp, which actually may be more efficient. However, I'm not
sure why others haven't done so already, I'll have to take a look.

Are you compiling 32bit or 64bit? I don't have a mingw setup at the moment, if
you change the cast to static_cast<int32_t const volatile&>(*addr) does it work?
This may be due to mingw gcc not being entirely LLP64.

[cjacek, 2013-06-20 14:40:40]

On 2013/06/20 14:30:59, bungeman1 wrote:
> you compiling 32bit or 64bit? I don't have a mingw setup at the moment, if
> you change the cast to static_cast<int32_t const volatile&>(*addr) does it
work?
> This may be due to mingw gcc not being entirely LLP64.

I tried both 32 and 64-bit builds and current code fails on both. When I change
it to int32_t it works on both setups. That seems like a good solution to me,
thanks!

[bungeman-skia, 2013-06-20 16:54:14]

Since I don't have a mingw setup, please verify that Patch Set 4 works for you.
Previous comments imply that this should work, but I would like to verify before
committing.

[cjacek, 2013-06-20 17:13:30]

On 2013/06/20 16:54:14, bungeman1 wrote:
> Since I don't have a mingw setup, please verify that Patch Set 4 works for
you.
> Previous comments imply that this should work, but I would like to verify
before
> committing.

Yes, it works. Thank you.

[bungeman-skia, 2013-06-20 17:54:29]

Committed patchset #4 manually as r9706 (presubmit successful).