1315274 - rename mozilla::MakeRange to mozilla::IntegerRange or similar

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

We have mozilla::MakeRange:

https://dxr.mozilla.org/mozilla-central/source/mfbt/IntegerRange.h#157

for use in ranged-for loops:

  for (auto i : MakeRange(length)) { ... }

but in experimenting with using mozilla::Range more widely, it seems useful to have mozilla::MakeRange be instead:

template<typename T>
Range<T>
MakeRange(T* aPtr, size_t aLength)
{
  return Range<T>(aPtr, aLength);
}

so one doesn't have to type out the template parameter when one wants to construct a Range.  Note that gsl::span<T> doesn't have this sort of convenience function, for better or worse.

If that seems reasonable, then we'd need to rename the current mozilla::MakeRange to something else.  Plain ol' mozilla::IntegerRange, perhaps?  Waldo, WDYT?

Comment 1

[Jeff Walden [:Waldo]]

That seems fair.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: rename mozilla::MakeRange to mozilla::IntegerRange

MakeRange is just way too generic for this sort of thing.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8808283 [details] [diff] [review]
rename mozilla::MakeRange to mozilla::IntegerRange

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

::: dom/base/nsDocument.cpp
@@ +10735,1 @@
>      exitDocs[i]->CleanupFullscreenState();

Is it feasible to convert this to

  for (auto& doc : exitDocs)

?  (or maybe const auto&)

::: dom/quota/ActorsParent.cpp
@@ +4388,1 @@
>      if (origins[index]) {

for (auto& origin : origins)?

::: js/src/gc/Marking.cpp
@@ +47,5 @@
>  using mozilla::ArrayLength;
>  using mozilla::DebugOnly;
>  using mozilla::IsBaseOf;
>  using mozilla::IsSame;
> +using mozilla::IntegerRange;

Reorder this alphabetically.

::: js/src/gc/Statistics.cpp
@@ +808,1 @@
>          PodArrayZero(phaseTimes[d]);

for (auto& phaseTime : phaseTimes)?

@@ +935,1 @@
>          sum += times[i][phase];

for (auto& phaseTime : times)?

@@ +982,2 @@
>          for (int i = 0; i < PHASE_LIMIT; i++)
>              phaseTotals[j][i] += phaseTimes[j][i];

Add braces around the outer loop while you're here.

::: js/src/gc/Statistics.h
@@ +294,1 @@
>                  mozilla::PodArrayZero(phaseTimes[i]);

for (auto& phaseTime : phaseTimes)?  I never remember the ordering of outer/inner dimensions in T[x][y], and that's what's being zeroed here.

::: mfbt/tests/TestIntegerRange.cpp
@@ +9,5 @@
>  
>  #include <stddef.h>
>  
>  using mozilla::IsSame;
> +using mozilla::IntegerRange;

Alphabetize.

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> ::: dom/base/nsDocument.cpp
> @@ +10735,1 @@
> >      exitDocs[i]->CleanupFullscreenState();
> 
> Is it feasible to convert this to
> 
>   for (auto& doc : exitDocs)
> 
> ?  (or maybe const auto&)
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +4388,1 @@
> >      if (origins[index]) {
> 
> for (auto& origin : origins)?

I don't know, and I tend to avoid ranged-for in DOM code, since it's not always obvious when JS runs.

> ::: js/src/gc/Statistics.cpp
> @@ +808,1 @@
> >          PodArrayZero(phaseTimes[d]);
> 
> for (auto& phaseTime : phaseTimes)?

Yup.

> @@ +935,1 @@
> >          sum += times[i][phase];
> 
> for (auto& phaseTime : times)?

Sadly, you can't ranged-for over a single dimension of multi-dimensional arrays; see [iterator.range].  I guess 

I skipped other |auto&|-ings because of this.
 
> @@ +982,2 @@
> >          for (int i = 0; i < PHASE_LIMIT; i++)
> >              phaseTotals[j][i] += phaseTimes[j][i];
> 
> Add braces around the outer loop while you're here.

Done.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0ce9941af5
rename mozilla::MakeRange to mozilla::IntegerRange; r=Waldo

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a0ce9941af5