Fix SkTFits in to work properly with signed/unsigned mixtures.

R=mtklein@google.com

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

[bungeman-skia, 2013-07-03 19:35:40]

Due to the amount of meta-programming going on here, we may need to split up the
overly general 'SkTemplates.h'. In particular, this code may need to go
somewhere new. However, please give this code a look and see if it makes any
sense.

[reed1, 2013-07-08 12:38:44]

This looks like a lot of code. Do we need to reference boost?
Do we really need all of this in a public Skia header?

[mtklein, 2013-07-08 13:39:39]

On 2013/07/08 12:38:44, reed1 wrote:
> This looks like a lot of code. Do we need to reference boost?
> Do we really need all of this in a public Skia header?

My instinct here is that this code is unnecessarily powerful.  SkTFitsIn is only
used in 3 places, in each to check we can fit a file size (of 3 different types)
into size_t.  I think I'd go the other way here and remove SkTFitsIn and replace
it with something more specific, maybe like this:

// If value is small enough to fit in size_t, write it into out and return true,
otherwise return false.
template <typename T>
inline bool SkTFitsInSizeT(T value, size_t* out) {
    const size_t max_size_t = ~static_cast<size_t>(0);
    if (value > max_size_t) return false;
    *out = static_cast<size_t>(value);
    return true;
}

Given that we're using this only to check file sizes, I don't think we have to
worry about signed / unsigned conversions, or really any negative values at all.
 If we're feeling pedantic I think a "if (value < 0) return false;" as the first
line should cover it.

[bungeman-skia, 2013-07-08 15:48:46]

On 2013/07/08 12:38:44, reed1 wrote:
> This looks like a lot of code. Do we need to reference boost?

The boost numeric_cast, on which this is loosely based, does not return a
boolean, is used something like

S s = ...;
D d = numeric_cast<D>(s);

and it throws an exception if (what is here called) SkTFitsIn<D>(s) would return
false. numeric_cast does provide an overflow handler, and could be made to work
without using exceptions, but it wouldn't be very nice. Also, I have some doubts
about the implementation there; in writing this there are some things they are
doing which are unclear.

> Do we really need all of this in a public Skia header?

No, I could just as well put this elsewhere (as I was suggesting above).
Probably in src/utils as it's currently used in ports and utils. I only really
started this here because a) I didn't expect it to be this big or this difficult
to get right and b) I believe that having this generally available will make it
more likely that we (efficiently) check values which may be out of range when
appropriate.

[bungeman-skia, 2013-07-10 19:05:12]

On 2013/07/08 13:39:39, mtklein wrote:
> On 2013/07/08 12:38:44, reed1 wrote:
> > This looks like a lot of code. Do we need to reference boost?
> > Do we really need all of this in a public Skia header?
> 
> My instinct here is that this code is unnecessarily powerful.  SkTFitsIn is
only
> used in 3 places, in each to check we can fit a file size (of 3 different
types)
> into size_t.  I think I'd go the other way here and remove SkTFitsIn and
replace
> it with something more specific, maybe like this:
> 
> // If value is small enough to fit in size_t, write it into out and return
true,
> otherwise return false.
> template <typename T>
> inline bool SkTFitsInSizeT(T value, size_t* out) {
>     const size_t max_size_t = ~static_cast<size_t>(0);
>     if (value > max_size_t) return false;
>     *out = static_cast<size_t>(value);
>     return true;
> }
> 

Except that this gives a warning (as error) about mixed sign comparison when T
is signed. We could pragma that away (as this should be correct anyway) but this
proposal doesn't need any pragmas and works in general. It would be interesting
to in addition have a version which debug asserts that the value is always
non-negative and doesn't check the low side in signed to unsigned conversions.
It's a fairly simple change, but I'm not sure about it.

> Given that we're using this only to check file sizes, I don't think we have to
> worry about signed / unsigned conversions, or really any negative values at
all.
>  If we're feeling pedantic I think a "if (value < 0) return false;" as the
first
> line should cover it.

[bungeman-skia, 2013-07-10 22:06:17]

At Patch Set 9, this works on all the platforms and tests what it is supposed to
test.

I believe this is superior to adding check methods on an ad-hoc basis (or
checking inline), since this

a) Takes care of all possible cases. It will not compile if not applicable (and
on clang and newer gcc, with a fairly readable error message).

b) Has tests written for it. If checks are written inline they will not have
direct tests (so it's easy to get them subtly wrong). If check methods are
written on an ad-hoc basis, we'll end up not writing tests for all of them
(which will be difficult to notice).

c) Because the compiler is generating the code at compile time, there is no need
to add ad-hoc check methods just because some platform uses a different type for
a typedef. There is also no need to evaluate if one of these ad-hoc methods is
still in use (it's easy to remove the need for such a method and not notice that
it is no longer used).

https://codereview.chromium.org/18503009/diff/18001/src/utils/win/SkDWriteFon...
File src/utils/win/SkDWriteFontFileStream.cpp (right):

https://codereview.chromium.org/18503009/diff/18001/src/utils/win/SkDWriteFon...
src/utils/win/SkDWriteFontFileStream.cpp:16: #include <limits>
After converting the other one, this can be removed.

Done

https://codereview.chromium.org/18503009/diff/18001/src/utils/win/SkDWriteFon...
src/utils/win/SkDWriteFontFileStream.cpp:187: if (fileOffset + fragmentSize >
(std::numeric_limits<size_t>::max)()) {
Missed this one.

Done

[mtklein, 2013-07-11 14:56:57]

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h
File src/utils/SkTFitsIn.h (right):

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h#new...
src/utils/SkTFitsIn.h:99: template<typename D, typename S> struct
SkTFitsIn_Unsigned2Unsiged {
Can you add a comment for each of these four reminding us why we need to do the
particular tests we do in each of these cases?

Or actually, is it possible to just over-check these guys?  Can we always just
do FullCheck, or does that bring us back to tautological warnings?

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h#new...
src/utils/SkTFitsIn.h:164: }
Do you mind tacking on
  // namespace Private
  // namespace sktfitsin
here?  Makes it easier to follow along which namespace you're in.

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTLogic.h
File src/utils/SkTLogic.h (right):

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTLogic.h#newc...
src/utils/SkTLogic.h:10: 
Some sort of high-level what's-going-on-here would be nice.

https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp
File tests/FitsInTest.cpp (right):

https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp#newc...
tests/FitsInTest.cpp:16: #define TEST(S, s, D, expected)
REPORTER_ASSERT(reporter, (SkTFitsIn<D>((S)(s)) == (expected)));
If you leave off the trailing ';' here, it'll force you to add it consistently
below.  :)

https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp#newc...
tests/FitsInTest.cpp:69: //TEST(float, 1, uint64_t, true);
Can you tack on a reminder why this is commented out?  I guess // We don't
support floating point types. ?

[bungeman-skia, 2013-07-11 20:22:03]

Patch Set 10 hopefully addresses all of the comments on Patch Set 9.

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h
File src/utils/SkTFitsIn.h (right):

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h#new...
src/utils/SkTFitsIn.h:99: template<typename D, typename S> struct
SkTFitsIn_Unsigned2Unsiged {
On 2013/07/11 14:56:57, mtklein wrote:
> Can you add a comment for each of these four reminding us why we need to do
the
> particular tests we do in each of these cases?

Hmmm... yes. So the reason I have for these tests isn't really well commented
here, but it is to ensure that we're satisfying the preconditions of the
OutOfRange_XXX types. For example SkTOutOfRange_GT_MaxD states that it assumes
that Max(S) >= Max(D). The reason for this assumption is that it simplifies the
assumptions making the following test valid:

return s > static_cast<S>((std::numeric_limits<D>::max)());

If we assume Max(S) >= Max(D) then the cast will always be non-narrowing and
simply prevents sign conversions.

If this assumption doesn't hold (and Max(S) > Max(D)), then the cast is
difficult to reason about (and the test provided by SkTOutOfRange_GT_MaxD isn't
needed anyway). If D is unsigned and S is signed, this is technically undefined
behavior (since unsigned Max(D) won't fit into signed S). If both are signed, I
think this is also technically undefined behavior. Given that both gcc and clang
are more and more taking advantage of (and propagating) undefined behavior,
allowing such code to be produced seems quite shaky.

I will make the preconditions of OutOfRange_XXX types SK_COMPILE_ASSERTs, and
comment to clarify.

> 
> Or actually, is it possible to just over-check these guys?  Can we always just
> do FullCheck, or does that bring us back to tautological warnings?

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h#new...
src/utils/SkTFitsIn.h:164: }
On 2013/07/11 14:56:57, mtklein wrote:
> Do you mind tacking on
>   // namespace Private
>   // namespace sktfitsin
> here?  Makes it easier to follow along which namespace you're in.

Done.

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTLogic.h
File src/utils/SkTLogic.h (right):

https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTLogic.h#newc...
src/utils/SkTLogic.h:10: 
On 2013/07/11 14:56:57, mtklein wrote:
> Some sort of high-level what's-going-on-here would be nice.

Sure, added comment that this is basically providing what C++11 will provide
with the type_traits header. Also, edited a few things to make sure that
everything is more or less drop-in replaceable with type_traits.

https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp
File tests/FitsInTest.cpp (right):

https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp#newc...
tests/FitsInTest.cpp:16: #define TEST(S, s, D, expected)
REPORTER_ASSERT(reporter, (SkTFitsIn<D>((S)(s)) == (expected)));
On 2013/07/11 14:56:57, mtklein wrote:
> If you leave off the trailing ';' here, it'll force you to add it consistently
> below.  :)

Done.

https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp#newc...
tests/FitsInTest.cpp:69: //TEST(float, 1, uint64_t, true);
On 2013/07/11 14:56:57, mtklein wrote:
> Can you tack on a reminder why this is commented out?  I guess // We don't
> support floating point types. ?

Done. Oops, yes, thought I had a comment here already. It would be interesting
to have a test setup where we could ensure that various bits of code don't
compile.

[mtklein, 2013-07-11 21:08:01]

On 2013/07/11 20:22:03, bungeman1 wrote:
> Patch Set 10 hopefully addresses all of the comments on Patch Set 9.
> 
> https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h
> File src/utils/SkTFitsIn.h (right):
> 
>
https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h#new...
> src/utils/SkTFitsIn.h:99: template<typename D, typename S> struct
> SkTFitsIn_Unsigned2Unsiged {
> On 2013/07/11 14:56:57, mtklein wrote:
> > Can you add a comment for each of these four reminding us why we need to do
> the
> > particular tests we do in each of these cases?
> 
> Hmmm... yes. So the reason I have for these tests isn't really well commented
> here, but it is to ensure that we're satisfying the preconditions of the
> OutOfRange_XXX types. For example SkTOutOfRange_GT_MaxD states that it assumes
> that Max(S) >= Max(D). The reason for this assumption is that it simplifies
the
> assumptions making the following test valid:
> 
> return s > static_cast<S>((std::numeric_limits<D>::max)());
> 
> If we assume Max(S) >= Max(D) then the cast will always be non-narrowing and
> simply prevents sign conversions.
> 
> If this assumption doesn't hold (and Max(S) > Max(D)), then the cast is
> difficult to reason about (and the test provided by SkTOutOfRange_GT_MaxD
isn't
> needed anyway). If D is unsigned and S is signed, this is technically
undefined
> behavior (since unsigned Max(D) won't fit into signed S). If both are signed,
I
> think this is also technically undefined behavior. Given that both gcc and
clang
> are more and more taking advantage of (and propagating) undefined behavior,
> allowing such code to be produced seems quite shaky.
> 
> I will make the preconditions of OutOfRange_XXX types SK_COMPILE_ASSERTs, and
> comment to clarify.
> 
> > 
> > Or actually, is it possible to just over-check these guys?  Can we always
just
> > do FullCheck, or does that bring us back to tautological warnings?
> 
>
https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTFitsIn.h#new...
> src/utils/SkTFitsIn.h:164: }
> On 2013/07/11 14:56:57, mtklein wrote:
> > Do you mind tacking on
> >   // namespace Private
> >   // namespace sktfitsin
> > here?  Makes it easier to follow along which namespace you're in.
> 
> Done.
> 
> https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTLogic.h
> File src/utils/SkTLogic.h (right):
> 
>
https://codereview.chromium.org/18503009/diff/14006/src/utils/SkTLogic.h#newc...
> src/utils/SkTLogic.h:10: 
> On 2013/07/11 14:56:57, mtklein wrote:
> > Some sort of high-level what's-going-on-here would be nice.
> 
> Sure, added comment that this is basically providing what C++11 will provide
> with the type_traits header. Also, edited a few things to make sure that
> everything is more or less drop-in replaceable with type_traits.
> 
> https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp
> File tests/FitsInTest.cpp (right):
> 
>
https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp#newc...
> tests/FitsInTest.cpp:16: #define TEST(S, s, D, expected)
> REPORTER_ASSERT(reporter, (SkTFitsIn<D>((S)(s)) == (expected)));
> On 2013/07/11 14:56:57, mtklein wrote:
> > If you leave off the trailing ';' here, it'll force you to add it
consistently
> > below.  :)
> 
> Done.
> 
>
https://codereview.chromium.org/18503009/diff/14006/tests/FitsInTest.cpp#newc...
> tests/FitsInTest.cpp:69: //TEST(float, 1, uint64_t, true);
> On 2013/07/11 14:56:57, mtklein wrote:
> > Can you tack on a reminder why this is commented out?  I guess // We don't
> > support floating point types. ?
> 
> Done. Oops, yes, thought I had a comment here already. It would be interesting
> to have a test setup where we could ensure that various bits of code don't
> compile.

LGTM

Thanks, this all really did help my comprehension.  Still feels to me like the
solution's more complicated than the problem, but putting that aside, this LGTM.

[bungeman-skia, 2013-07-12 18:22:52]

Committed patchset #10 manually as r10050.