1091882 - PodOperation.h's uses of mozilla::PointerRangeSize could be simplified

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jim Blandy :jimb]

Attachment: Simplify some uses of mozilla::PointerRangeSize, and clarify comment.

PodOperations.h uses mozilla::PointerRangeSize when a shorter and more legible comparison would do. Also, the comment for mozilla::PointerRangeSize is confusing.

Comment 1

[Jim Blandy :jimb]

Try push (build only):
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f7203e892e4a

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8514583 [details] [diff] [review]
Simplify some uses of mozilla::PointerRangeSize, and clarify comment.

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

::: mfbt/ArrayUtils.h
@@ +27,5 @@
>  /*
> + * Safely subtract two pointers when it is known that aEnd >= aBegin.
> + *
> + * Most compilers get the sign of |aEnd - aBegin| wrong when |sizeof(T) > 1| and
> + * |((char *) aEnd - (char *) aBegin)| doesn't fit in a ptrdiff_t.

Could you move most of this comment, into a comment inside PointerRangeSize itself?  This is interesting, but it's far, far more detail than is valuable as part of interface documentation, that the unfamiliar reader will likely read in its entirety to familiarize himself with the function.  Most of this is not important to such a reader; he only wants to know what the method does.  But he has no way of knowing that without reading it all.

And, frankly, even inside this method I'm not entirely sure all the rationale for compiler bugs is necessary.  Perhaps better to say that pointer subtraction produces a signed ptrdiff_t value and that subtraction of sufficiently-separated pointers may overflow the ptrdiff_t space (either in the final value, or in the implicit intermediate step before translating address-space delta into an element count), and leave it at that.

::: mfbt/PodOperations.h
@@ +81,5 @@
>  template<typename T>
>  static MOZ_ALWAYS_INLINE void
>  PodAssign(T* aDst, const T* aSrc)
>  {
> +  MOZ_ASSERT(aDst + 1 <= aSrc || aSrc + 1 <= aDst);

MOZ_ASSERT(aDst + 1 <= aSrc || aSrc + 1 <= aDst,
           "destination and source must not overlap");

@@ +94,5 @@
>  template<typename T>
>  static MOZ_ALWAYS_INLINE void
>  PodCopy(T* aDst, const T* aSrc, size_t aNElem)
>  {
> +  MOZ_ASSERT(aDst + aNElem <= aSrc || aSrc + aNElem <= aDst);

"destination and source ranges must not overlap"

@@ +112,5 @@
>  template<typename T>
>  static MOZ_ALWAYS_INLINE void
>  PodCopy(volatile T* aDst, const volatile T* aSrc, size_t aNElem)
>  {
> +  MOZ_ASSERT(aDst + aNElem <= aSrc || aSrc + aNElem <= aDst);

"destination and source ranges must not overlap"

Comment 3

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6902b39cd26e

I changed the comment to:

+ * Safely subtract two pointers when it is known that aEnd >= aBegin, yielding a
+ * size_t result.
+ *
+ * Ordinary pointer subtraction yields a ptrdiff_t result, which, being signed,
+ * has insufficient range to express the distance between pointers at opposite
+ * ends of the address space. Furthermore, most compilers use ptrdiff_t to
+ * represent the intermediate byte address distance, before dividing by
+ * sizeof(T); if that intermediate result overflows, they'll produce results
+ * with the wrong sign even when the correct scaled distance would fit in a
+ * ptrdiff_t.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6902b39cd26e