835542 - Implement mozilla::Abs

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

std::abs doesn't work on every type -- in particular, it doesn't work on |long long| (you're supposed to use std::llabs instead).  The type underlying |int64_t| varies from platform to platform, and on some platforms it's |long long|.  Thus any use of |std::abs(int64_t)| is a landmine, and we should implement a mozilla::Abs in MathAlgorithms.h that works at least on all signed natural types and on all signed <stdint.h> types.

As the natural types will almost always overlap the <stdint.h> types, this will require something similar to the detail::IsSupported stuff to expose all the right overloads, and no others.

Comment 1

[Jeff Walden [:Waldo]]

The detail::IsSupported stuff in CheckedInt.h, that is.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Add IntegralConstant, TrueType, FalseType helpers to TypeTraits.h

static const bool value = true; is getting very old.  Plus, technically that's not even <type_traits>-compatible, although I kind of doubt too many people are, or should, depend on the extra stuff.

Note that for brevity, in many places I have this:

struct S : Foo

which, because of the 'struct' keyword instead of the 'class' keyword, means Foo gets inherited publicly.  I could add 'public' in for explicitness, to be sure, but given the whole idea of this is to cut down typing, I kind of like leaving it off for at least code in mfbt.  (Others may think differently for their own code, hence my having public in the other bits.)

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Implement mozilla::Abs, with overloads for all the signed natural types, and for float/double/long double

This is mostly pretty dreary.  I was able to eliminate a little complexity where people were already hacking around the abs(int64_t) issue already, and to eliminate a place using the C overload -- seems a win to me to not require people to think about exactly what overload they're calling.

Oh, and the IsAbsSupported stuff is where the TrueType/FalseType stuff from the previous patch comes into play, obviously.

I tryservered both patches here:

https://tbpl.mozilla.org/?tree=Try&rev=eb11d3369338

...and again on Windows here when I discovered Windows code actually wants the long double overload:

https://tbpl.mozilla.org/?tree=Try&rev=cf7b616bc514

(long double is in C++98, so I don't think I need to tryserver that addition again, for what it's worth.)

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 714762 [details] [diff] [review]
Implement mozilla::Abs, with overloads for all the signed natural types, and for float/double/long double

Review of attachment 714762 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3679,5 @@
>    int32_t wi = JS_DoubleToInt32(sw);
>    int32_t hi = JS_DoubleToInt32(sh);
>  
> +  uint32_t w = mozilla::Abs(wi);
> +  uint32_t h = mozilla::Abs(hi);

No need for the mozilla::. Please drop it in all non-js files except for nsCSSColorUtils.h.

::: editor/libeditor/html/nsHTMLObjectResizer.cpp
@@ +875,5 @@
>      int32_t yThreshold =
>        LookAndFeel::GetInt(LookAndFeel::eIntID_DragThresholdY, 1);
>  
> +    if (mozilla::Abs(clientX - mOriginalX ) * 2 >= xThreshold ||
> +        mozilla::Abs(clientY - mOriginalY ) * 2 >= yThreshold) {

Fix the space before the ) while you're here.

::: js/src/ion/RangeAnalysis.cpp
@@ +617,5 @@
>      EnsureRange(&other);
>  
>      Range *range = new Range(0,
> +                             Max(mozilla::Abs(int64_t(other->lower())),
> +                                 mozilla::Abs(int64_t(other->upper()))));

Consider Abs<int64_t>(foo)

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +2524,4 @@
>  void
>  MacroAssemblerARMCompat::storeValue(ValueOperand val, const BaseIndex &dest)
>  {
> +    if (isValueDTRDCandidate(val) && (mozilla::Abs(dest.offset) <= 255)) {

Feel free to drop the parens here and below

::: layout/tables/nsTableFrame.cpp
@@ +6247,4 @@
>    if (!haveIntersect)
>      return false;
>    mDamageArea = nsIntRect(startColIndex, startRowIndex,
> +                          1 + mozilla::Abs(int32_t(endColIndex - startColIndex)),

Same here

::: mfbt/MathAlgorithms.h
@@ +99,5 @@
> +  // won't fit in that type (on twos-complement systems), so assert that the
> +  // input isn't that value.
> +  MOZ_ASSERT(t >= 0 ||
> +             -(t + 1) != T((1ULL << (CHAR_BIT * sizeof(T) - 1)) - 1),
> +             "You can't negate the smallest possible negative integer!");

I'm not going to claim I understand this

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 714761 [details] [diff] [review]
Add IntegralConstant, TrueType, FalseType helpers to TypeTraits.h

Review of attachment 714761 [details] [diff] [review]:
-----------------------------------------------------------------

All I can do here is stare blankly and nod. If you want a real review, I suggest asking someone who knows more about templates :)

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 714761 [details] [diff] [review]
Add IntegralConstant, TrueType, FalseType helpers to TypeTraits.h

Review of attachment 714761 [details] [diff] [review]:
-----------------------------------------------------------------

bjacob, this is somewhat some nitpickery about |value| not being sufficient to fill out so-called UnaryTypeTrait stuff from 20.9.3 in C++11, and somewhat getting rid of typing |static const bool value = true;| everywhere.  :-)  As noted in a previous comment, be aware that |struct S : T| is specified to inherit T publicly -- seems to me that much less typing is a nice thing here.

The BaseOf code motion is partly to get the TrueType/FalseType inheritance correct, and partly to make everything a struct, so that the less-typing thing can be applied.

Given the traits we have so far, I have no doubt more are coming even beyond this that'll justify TrueType and FalseType.  O brave new world, That has such templates in't!

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 714762 [details] [diff] [review]
Implement mozilla::Abs, with overloads for all the signed natural types, and for float/double/long double

Review of attachment 714762 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/MathAlgorithms.h
@@ +99,5 @@
> +  // won't fit in that type (on twos-complement systems), so assert that the
> +  // input isn't that value.
> +  MOZ_ASSERT(t >= 0 ||
> +             -(t + 1) != T((1ULL << (CHAR_BIT * sizeof(T) - 1)) - 1),
> +             "You can't negate the smallest possible negative integer!");

Abs(int8_t(-128)) is bad, because it's impossible to represent 128 in an int8_t.  So any time the minimum signed value comes in is an error.  One of two things must hold for there to be no error (in a twos-complement world, which this code somewhat blithely assumes).

The number can be non-negative, which is easy (and the first alternative).

Or, the number can be negative but not the minimum (the second).  We don't have a way to compare directly to the minimum value, because std::numeric_limits<> isn't guaranteed to be specialized for <stdint.h> types.  So we do a little trickery.  Negation is sensible if the value *isn't* the min-value.  So add one, so *if* it were the min-value, it no longer is.  Now negate, safely.  This gives us a value from 0 to the max signed value.  Any value but the min-signed-value as input would give a value less than the max signed value here.  So we can compare -(t + 1) to the max signed value -- if it's equal, we have a bad value.  So check that it's not.  T(...) can be broken down like this:

* the max signed value is going to be 2**(width - 1) - 1, cast to T
* width is sizeof(T) * width of a char
* which gives us the (1 << (CHAR_BIT * sizeof(T) - 1)) - 1, cast to T

I'll add a comment of some sort to try to clarify this somewhat.

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 714761 [details] [diff] [review]
Add IntegralConstant, TrueType, FalseType helpers to TypeTraits.h

Review of attachment 714761 [details] [diff] [review]:
-----------------------------------------------------------------

I have just enough questions/comments to require a second round. Sorry about the terribly delayed reviews.

::: js/public/HashTable.h
@@ +556,5 @@
>  
>  template <class T>
>  struct IsPod<js::detail::HashTableEntry<T> >
> +  : public IntegralConstant<bool, IsPod<T>::value>
> +{};

While you're at it, why not just inherit IsPod<T> ?

::: js/src/frontend/ParseMaps.h
@@ +421,4 @@
>  namespace mozilla {
>  
>  template <>
> +struct IsPod<js::frontend::DefinitionList> : public TrueType {};

Stray public keyword here.

::: js/src/jsanalyze.h
@@ +1296,5 @@
> +template <> struct IsPod<js::analyze::LifetimeVariable> : public TrueType {};
> +template <> struct IsPod<js::analyze::LoopAnalysis>     : public TrueType {};
> +template <> struct IsPod<js::analyze::SlotValue>        : public TrueType {};
> +template <> struct IsPod<js::analyze::SSAValue>         : public TrueType {};
> +template <> struct IsPod<js::analyze::SSAUseChain>      : public TrueType {};

More stray public keywords.

::: mfbt/TypeTraits.h
@@ +71,5 @@
> +
> +  public:
> +    static const bool value =
> +      sizeof(test(BaseOfHelper<Base, Derived>(), int())) == sizeof(char);
> +};

I can't review this to the point of checking for possible errors, but this is eminently testable and we already have some tests in TestTypeTraits.cpp. Please just expand it a little bit for the following cases:
 - const (esp. as you have the above partial specialization just for this case)
 - pointer types
 - reference types (esp. as you have custom code just for that below: )
 - volatile just for kicks?

@@ +74,5 @@
> +      sizeof(test(BaseOfHelper<Base, Derived>(), int())) == sizeof(char);
> +};
> +
> +template<class Base, class Derived>
> +struct BaseOfTester<Base&, Derived&> : FalseType {};

I don't get why you need this for reference types and not for pointer types?

@@ +238,5 @@
> +template<> struct IsPod<bool>               : TrueType {};
> +template<> struct IsPod<float>              : TrueType {};
> +template<> struct IsPod<double>             : TrueType {};
> +template<> struct IsPod<wchar_t>            : TrueType {};
> +template<typename T> struct IsPod<T*>       : TrueType {};

So what about IsPod<int8_t> and other stdint types that may not match above types? Currently they'll be reported as non-pod AIUI. Here too it seems like you need a two-pass IsSupported-like idiom.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Benoit Jacob [:bjacob] from comment #8)
> I have just enough questions/comments to require a second round. Sorry about
> the terribly delayed reviews.

No worries, I'm so far behind on mine it really doesn't matter to me.  It's an easy patch to carry along.

> While you're at it, why not just inherit IsPod<T> ?

Good idea.

> Stray public keyword here.
> More stray public keywords.

This was somewhat because explicit "public" is possibly more in line with SpiderMonkey style.  But for this it probably doesn't matter all that much.

> I can't review this to the point of checking for possible errors, but this
> is eminently testable and we already have some tests in TestTypeTraits.cpp.

The IsBaseOf changes are purely code-motion to get to the point where mozilla::IsBaseOf inherits from IntegralConstant<bool, true or false> as appropriate.  There should be no change to IsBaseOf<T>::value for any type; I probably could have made that clearer.  Given that -- and also given that IsBaseOf isn't really fully to spec, and we'll align it more if/when we need it, are you okay with the patch if I make the changes above but don't add to IsBaseOf tests, or further tweak IsBaseOf semantics?

> So what about IsPod<int8_t> and other stdint types that may not match above
> types? Currently they'll be reported as non-pod AIUI. Here too it seems like
> you need a two-pass IsSupported-like idiom.

Technically, yes.  Every <stdint.h> type is one of the fundamental types with the compilers we use, however, so for IsPod which is purely an optimization we can sort of get away with it.  (CheckedInt thinks int8_t is special, but actually that's just because |signed char| isn't explicitly supported -- a change for another bug.)  Unfortunately there's not really a clean way to handle this, because StandardInteger.h and TypeTraits.h can be included in two orders, and TypeTraits.h can't include StandardInteger.h without changing the TypeTraits.h interface.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

OK, that's fine -- you can go ahead.

re: int8_t and signed char: are you saying that signed char is a different type than char??

Comment 11

[Jeff Walden [:Waldo]]

Yes.  One of those backwards-compatibility things: char has the same representation as either unsigned char or signed char, but which one's chosen is implementation-defined.

Comment 12

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/645cf7ea0876
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8b3e397ffc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/645cf7ea0876
https://hg.mozilla.org/mozilla-central/rev/5a8b3e397ffc

Comment 14

[Jeff Walden [:Waldo]]

For anyone who's wondering why I haven't posted to newsgroups about this: I want to deal with bug 847480, one way or another (including auditing all callers, if a change is made), before making any sort of announcement/whatever in m.d.platform.