Closed Bug 1091882 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file)

PodOperations.h uses mozilla::PointerRangeSize when a shorter and more legible comparison would do. Also, the comment for mozilla::PointerRangeSize is confusing.
Attachment #8514583 - Flags: review?(jwalden+bmo)
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"
Attachment #8514583 - Flags: review?(jwalden+bmo) → review+
Component: JavaScript Engine → MFBT
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.
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Flags: in-testsuite-
Target Milestone: --- → mozilla36
https://hg.mozilla.org/mozilla-central/rev/6902b39cd26e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.