Closed Bug 1470985 Opened 2 years ago Closed 2 years ago

PodEqual should probably be removed

Categories

(Core :: MFBT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

Details

Attachments

(3 files)

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 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.
Attachment #8989326 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #8989326 - Flags: review?(jwalden+bmo) → review+
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 on attachment 8989326 [details]
Bug 1470985 - s/PodEqual/ArrayEqual/ from ArrayUtils.h. -

https://reviewboard.mozilla.org/r/254392/#review262022
Comment on attachment 8990140 [details]
Bug 1470985 - EqualChars should just call ArrayEqual. -

https://reviewboard.mozilla.org/r/255142/#review262330
Attachment #8990140 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8990141 [details]
Bug 1470985 - const_cast no longer necessary with ArrayEqual. -

https://reviewboard.mozilla.org/r/255144/#review262332
Attachment #8990141 - Flags: review?(jwalden+bmo) → review+
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)
Pushed by jgilbert@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.