Fix (1 << 31) to (1u << 31) in SkOTTable_OS_2.

Fix (1 << 31) to (1u << 31) in SkOTTable_OS_2.

When ints are 32 bits, (1 << 31) is undefined.

R=reed@google.com

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

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

[bungeman-skia, 2014-03-10 23:13:05]

I have opened skia:2285 for remaining cases.

[reed1, 2014-03-11 13:57:54]

Why must we change *all* of the shifts, and not just the tricky ones?
I wonder if a small macro to build constants from shifts would make this
readable (and safe)

e.g.

SkSetBit(unsigned bitIndex) { return 1U << bitIndex; }

[bungeman-skia, 2014-03-11 15:50:54]

On 2014/03/11 13:57:54, reed1 wrote:
> Why must we change *all* of the shifts, and not just the tricky ones?
> I wonder if a small macro to build constants from shifts would make this
> readable (and safe)
> 
> e.g.
> 
> SkSetBit(unsigned bitIndex) { return 1U << bitIndex; }

Well, these need to be compile time constants, and we don't have constexpr yet,
so it would need to look something like

template<unsigned N> struct SkTSetBit {
    static const uintmax_t value = UINTMAX_C(1) << N;
};

and replace '1u << N' with 'SkTSetBit<1>::value'. Not sure it's entirely more
clear, but using it does mean that one doesn't forget to make the LHS unsigned.
We'd want the function call version for runtime setting, though it may want to
be templated to account for types other than unsigned.

Note that we'll need to define __STDC_CONSTANT_MACROS before stdint.h, or switch
to cstdint.h to get UNINTMAX_C.

In other words, this change (PS1) is fast and easy, which is why I put it up.
I'll see what can be done.

[bungeman-skia, 2014-03-11 19:59:38]

At PS3 here is a nice safe way to go about this which is clear and will always
compile time error if we try to do something wrong.

[reed1, 2014-03-11 20:01:40]

lets also review this with klein when he's back.

lgtm

[bungeman-skia, 2014-03-12 03:14:23]

Committed patchset #3 manually as r13757.

[bungeman-skia, 2014-03-12 05:36:03]

> Committed patchset #3 manually as r13757.

Reverted r13758 due to odd vc++ behavior.

typedef SkCompileAssert<(bool(std::numeric_limits<T>::is_integer))>
msg[bool(std::numeric_limits<T>::is_integer) ? 1 : -1];
typedef SkCompileAssert<(bool(std::numeric_limits<T>::is_integer))>
msg[bool(bool(std::numeric_limits<T>::is_integer)) ? 1 : -1];

fail, but 

typedef SkCompileAssert<(bool(std::numeric_limits<T>::is_integer))>
msg[bool((std::numeric_limits<T>::is_integer)) ? 1 : -1];
typedef SkCompileAssert<(bool(std::numeric_limits<T>::is_integer))>
msg[static_cast<bool>(std::numeric_limits<T>::is_integer) ? 1 : -1];

are fine. Likewise

SK_COMPILE_ASSERT(std::numeric_limits<T>::is_integer,
SkTSetBit_T_must_be_integer);

fails, but

SK_COMPILE_ASSERT((std::numeric_limits<T>::is_integer),
SkTSetBit_T_must_be_integer);

is fine.

It's difficult to see what the exact trigger is. It does not appear to be
related to the default template argument, the sub-expression being used twice,
nor any spacing issues. Perhaps it's a rvalue/lvalue issue with is_integer being
a static from a base class? Because ::is_signed doesn't seem to have this issue,
but it's directly set on the class (hiding fields in base classes).

[mtklein, 2014-03-12 12:33:42]

Is there any way we can do this just big build-breaking warnings if things go
undefined?  (1<<N) and (1u<<N) where necessary are just so much clearer than
SkSomethingSetSomethingBit<N>::value that I'd like to keep them where possible.

[bungeman-skia, 2014-03-12 14:41:26]

On 2014/03/12 12:33:42, mtklein wrote:
> Is there any way we can do this just big build-breaking warnings if things go
> undefined?  (1<<N) and (1u<<N) where necessary are just so much clearer than
> SkSomethingSetSomethingBit<N>::value that I'd like to keep them where
possible.

Well, these are compile time constants, and I actually don't care that there's a
shift, in theory I could also write these like 0x0100u but I really do want to
express that I'm setting a specific bit in a BE layout integer. In other words,
I don't think that (1u << N) expresses intent either, it just computes 2^N. In
theory, I should add another assert that (std::numeric_limits<T>::radix == 2)
because that's also a requirement (of course we'd break all over the place if
this weren't true anyway, the spec actually has this for all those IBM machines
which only have hardware BCD and no binary integers at all). The reason for the
SkOT versions isn't just to do the shift (be a wrapper), it's to set the bit on
the BE representation (SK_OT_USHORT and ULONG are BE types). I don't expect that
other users will need this sort of 'wrapping' at all.

That being said, even clang didn't actually seem to be warning about the
undefined shift, it was actually carrying out the undefined shift but then
complaining that the resulting negative value didn't fit into the uint32_t the
function call expected. I'm afraid that in the future this sort of undefined
compile time behavior will end up propagated. In fact the entire reason for most
of this is that no compiler was giving big build breaking warnings on our
undefined constant shifts, and some of the rest is because only gcc currently
seems to catch some of the overflows (and we force build failure on all
platforms in these cases with this change).

[bungeman-skia, 2014-03-12 20:15:31]

I've opened https://connect.microsoft.com/VisualStudio/feedback/details/832915/
for what looks to be the vc++ bug which caused the Windows failures.

[bungeman-skia, 2014-03-12 20:59:08]

@reed: could you take a quick look at the SkTypes.h change I'm proposing to work
around vc++?

[reed1, 2014-03-12 21:00:57]

adding Brian just in case, but lgtm

[bungeman-skia, 2014-03-12 21:41:12]

Committed patchset #5 manually as r13779 (presubmit successful).

[Stephen White, 2014-03-12 21:53:16]

On 2014/03/12 21:41:12, bungeman1 wrote:
> Committed patchset #5 manually as r13779 (presubmit successful).

Looks like this broke all the Linux builds. e.g.,

http://108.170.217.252:10117/builders/Perf-Ubuntu12-ShuttleA-ATI5770-x86-Rele...

[bungeman-skia, 2014-03-12 22:00:56]

On 2014/03/12 21:53:16, Stephen White wrote:
> On 2014/03/12 21:41:12, bungeman1 wrote:
> > Committed patchset #5 manually as r13779 (presubmit successful).
> 
> Looks like this broke all the Linux builds. e.g.,
> 
>
http://108.170.217.252:10117/builders/Perf-Ubuntu12-ShuttleA-ATI5770-x86-Rele...

Should be fixed with r13781. With all the testing and mucking about, forgot that
the parens that were around the first cast are required to prevent a '>' in the
'expr' from terminating the template type declaration. Of course, vc++ let this
slide.

[bungeman-skia, 2014-03-13 17:36:16]

On 2014/03/12 22:00:56, bungeman1 wrote:
> On 2014/03/12 21:53:16, Stephen White wrote:
> > On 2014/03/12 21:41:12, bungeman1 wrote:
> > > Committed patchset #5 manually as r13779 (presubmit successful).
> > 
> > Looks like this broke all the Linux builds. e.g.,
> > 
> >
>
http://108.170.217.252:10117/builders/Perf-Ubuntu12-ShuttleA-ATI5770-x86-Rele...
> 
> Should be fixed with r13781. With all the testing and mucking about, forgot
that
> the parens that were around the first cast are required to prevent a '>' in
the
> 'expr' from terminating the template type declaration. Of course, vc++ let
this
> slide.

Note that these extra parens were due to a bug in gcc, see
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57771 . (How about that, two
compiler bugs, one for gcc and one for vc++, from one line of code.)

I am proposing that this be commented at
https://codereview.chromium.org/196473013/ .