1470985 - PodEqual should probably be removed

Status: RESOLVED FIXED

(Core :: MFBT, enhancement)

Description

[Kelsey Gilbert [:jgilbert]]

Problems:
- Struct padding
- Floats might have NaNs that may-or-may-not be bitwise equal.
- bool in-memory representation?

We should probably just remove it. There's only a page of usages, which should be pretty easy to migrate off of.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

We can't use memcmp to compare PODs, largely because of undefined
padding. The rest of the Pod* functions are fine though, since we're
replicating or zeroing PODs.

Review commit: https://reviewboard.mozilla.org/r/254392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254392/

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

https://reviewboard.mozilla.org/r/254392/#review261386

Just remove PodEqual and use std::equal directly at the call sites.

Comment 3

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254392/diff/1-2/

Comment 4

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254392/diff/2-3/

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

https://reviewboard.mozilla.org/r/254392/#review261966

::: js/src/jsapi-tests/testExternalStrings.cpp:29
(Diff revision 3)
>  static const JSStringFinalizer finalizer2 = { finalize_str };
>  
>  static void
>  finalize_str(const JSStringFinalizer* fin, char16_t* chars)
>  {
> -    if (chars && PodEqual(const_cast<const char16_t*>(chars), arr, arrlen)) {
> +    if (chars && ArrayEqual(const_cast<const char16_t*>(chars), arr, arrlen)) {

Remove the const_cast<> once you change ArrayUtils.h as requested.

::: js/src/util/Text.h:77
(Diff revision 3)
>  {
>      for (const Char1* s1end = s1 + len; s1 < s1end; s1++, s2++) {
>          if (*s1 != *s2)
>              return false;
>      }
>      return true;

EqualChars(const Char1* s1, const Char2* s2, size_t len)'s body should just call std::equal.

And actually, at that point you don't need to have <typename Char1> and <typename Char1, typename Char2> specializations both -- you can just have the latter.

::: mfbt/ArrayUtils.h:110
(Diff revision 3)
> +
> +template<typename T, typename U, size_t N>
> +bool
> +ArrayEqual(const T (&a)[N], const U (&b)[N])
> +{
> +  return std::equal(a, a + N, b);

Remove the consts on these, and then const_cast<> the arguments passed to the function.  That lets us get rid of some const_cast<>s in various users, that exist only to adhere to the const-requirement.

::: mfbt/ArrayUtils.h:117
(Diff revision 3)
> +
> +template<typename T, typename U>
> +bool
> +ArrayEqual(const T* const a, const U* const b, const size_t n)
> +{
> +  return std::equal(a, a + n, b);

Same const_cast<> moving here too.

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1470985 - EqualChars should just call ArrayEqual. -

Review commit: https://reviewboard.mozilla.org/r/255142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255142/

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

https://reviewboard.mozilla.org/r/254392/#review261966

> Remove the consts on these, and then const_cast<> the arguments passed to the function.  That lets us get rid of some const_cast<>s in various users, that exist only to adhere to the const-requirement.

Switching to using a different typename U for b means const_cast is unnecessary.

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

https://reviewboard.mozilla.org/r/254392/#review262022

Comment 9

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1470985 - const_cast no longer necessary with ArrayEqual. -

Review commit: https://reviewboard.mozilla.org/r/255144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255144/

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8990140 [details]
Bug 1470985 - EqualChars should just call ArrayEqual. -

https://reviewboard.mozilla.org/r/255142/#review262330

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 8990141 [details]
Bug 1470985 - const_cast no longer necessary with ArrayEqual. -

https://reviewboard.mozilla.org/r/255144/#review262332

Comment 12

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s afb833fc98ed4fd8774085f5420543f673cf1be2 -d 9e0a8856d71a: rebasing 471614:afb833fc98ed "Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. - r=Waldo"
merging gfx/layers/LayerTreeInvalidation.cpp
merging js/src/shell/js.cpp
merging js/src/vm/JSAtom.cpp
merging js/src/vm/StringType.cpp
merging js/src/wasm/WasmTypes.h
warning: conflicts while merging gfx/layers/LayerTreeInvalidation.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cffca1854a03
s/PodEqual/ArrayEqual/ from ArrayUtils.h. - r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9047c36578
EqualChars should just call ArrayEqual. - r=waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/8902c2c4e98a
const_cast no longer necessary with ArrayEqual. - r=waldo

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/cffca1854a03
https://hg.mozilla.org/mozilla-central/rev/0f9047c36578
https://hg.mozilla.org/mozilla-central/rev/8902c2c4e98a