1385571 - Convert loops to use the range-based loops (C++11)

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

One more batch of updates
Generated with https://clang.llvm.org/extra/clang-tidy/checks/modernize-loop-convert.html
in some cases, I manually replaced the "i" variable by another "name"

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: loop.diff

Please let me know if you want me to split it and ask to individual reviewer instead.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Try job here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=749dcc50d10c12244cc543e210cdde46dc7df3e2

Comment 3

[Sylvestre Ledru [:Sylvestre]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4616c282e32f66850f398f4374e8dbb36702d90d

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Attachment: loop2.diff

Comment 5

[(no longer active)]

This patch is extremely large, it will take me a long time to get to it.  I'm pretty busy these days and this has a very low priority compared to the other things I have on my plate.  Sorry about that.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8891682 [details] [diff] [review]
loop2.diff

no worries ;)

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1385571 - Convert loops to use the range-based loops (C++11)

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

Comment 16

[Jean-Yves Avenard [:jya]]

Comment on attachment 8893333 [details]
Bug 1385571 - Convert loops to use the range-based loops (C++11)

https://reviewboard.mozilla.org/r/164424/#review169696

r- for now as SpeechDispatcherService.cpp is now broken.

::: dom/media/DecoderDoctorDiagnostics.cpp:468
(Diff revision 1)
>    // Report non-solved issues to console.
>    if (!aIsSolved) {
>      // Build parameter array needed by console message.
>      AutoTArray<const char16_t*,
>                 NotificationAndReportStringId::maxReportParams> params;
> -    for (int i = 0; i < NotificationAndReportStringId::maxReportParams; ++i) {
> +    for (auto mReportParam : aNotification.mReportParams) {

const auto&

otherwise this will do a copy (which didn't occur before).

The name also starts with a m which is reserved for class member.

please use reportParam

::: dom/media/MediaCache.cpp:975
(Diff revision 1)
>    // Now update references to blocks in block lists.
>    mFreeBlocks.NotifyBlockSwapped(aBlockIndex1, aBlockIndex2);
>  
>    nsTHashtable<nsPtrHashKey<MediaCacheStream> > visitedStreams;
>  
> -  for (int32_t i = 0; i < 2; ++i) {
> +  for (auto & block : blocks) {

auto&

but here, const auto& is better as it clearly show that the variable isn't written to in the loop (block being a pointer, its members can be written to)

::: dom/media/MediaCache.cpp:976
(Diff revision 1)
>    mFreeBlocks.NotifyBlockSwapped(aBlockIndex1, aBlockIndex2);
>  
>    nsTHashtable<nsPtrHashKey<MediaCacheStream> > visitedStreams;
>  
> -  for (int32_t i = 0; i < 2; ++i) {
> -    for (uint32_t j = 0; j < blocks[i]->mOwners.Length(); ++j) {
> +  for (auto & block : blocks) {
> +    for (uint32_t j = 0; j < block->mOwners.Length(); ++j) {

what about this one ?

for (auto&& owner : block->mOwners) {
 ...
}

::: dom/media/gtest/TestIntervalSet.cpp:215
(Diff revision 1)
>  
>    do {
>      do {
>        // Create intervals according to new indexes.
>        IntIntervals i_0;
> -      for (uint32_t i = 0; i < comb1.size(); i++) {
> +      for (unsigned int c : comb1) {

please use uint32_t (or size_t)

::: dom/media/gtest/TestIntervalSet.cpp:221
(Diff revision 1)
> -        i_0 += aI1[comb1[i]];
> +        i_0 += aI1[c];
>        }
>        // Test that intervals are always normalized.
>        Compare(aI1, i_0);
>        IntIntervals i_1;
> -      for (uint32_t i = 0; i < comb2.size(); i++) {
> +      for (unsigned int c : comb2) {

same here

::: dom/media/gtest/TestVPXDecoding.cpp:81
(Diff revision 1)
>    { "test_case_1224369.vp8.ivf", VPX_CODEC_CORRUPT_FRAME }
>  };
>  
>  TEST(libvpx, test_cases)
>  {
> -  for (size_t test = 0; test < ArrayLength(testFiles); ++test) {
> +  for (auto testFile : testFiles) {

const auto&

::: dom/media/gtest/TestVideoTrackEncoder.cpp:228
(Diff revision 1)
>      { true, 800, 480},    // Standard WVGA
>      { true, 960, 540},    // Standard qHD
>      { true, 1280, 720}    // Standard HD
>    };
>  
> -  for (size_t i = 0; i < ArrayLength(params); i++)
> +  for (auto param : params)

const auto&

::: dom/media/gtest/TestVideoTrackEncoder.cpp:246
(Diff revision 1)
>      { true, 800, 480},    // Standard WVGA
>      { true, 960, 540},    // Standard qHD
>      { true, 1280, 720}    // Standard HD
>    };
>  
> -  for (size_t i = 0; i < ArrayLength(params); i++)
> +  for (auto & param : params)

same

and & is to be attached to auto, as auto acts as a type

::: dom/media/mediasource/gtest/TestContainerParser.cpp:24
(Diff revision 1)
>      "video/mp4",
>      "audio/mp4",
>      "audio/aac"
>    };
>    nsAutoPtr<ContainerParser> parser;
> -  for (size_t i = 0; i < ArrayLength(containerTypes); ++i) {
> +  for (auto & cont : containerTypes) {

same

use containerType as name rather than cont

::: dom/media/ogg/OggDemuxer.cpp:473
(Diff revision 1)
>      { TrackInfo::kAudioTrack, TrackInfo::kVideoTrack };
>  
>    nsTArray<OggCodecState*> bitstreams;
>    nsTArray<uint32_t> serials;
>  
> -  for (uint32_t i = 0; i < ArrayLength(tracks); i++) {
> +  for (auto & track : tracks) {

same

::: dom/media/platforms/ffmpeg/FFmpegRuntimeLinker.cpp:56
(Diff revision 1)
>  
>    // While going through all possible libs, this status will be updated with a
>    // more precise error if possible.
>    sLinkStatus = LinkStatus_NOT_FOUND;
>  
> -  for (size_t i = 0; i < ArrayLength(sLibs); i++) {
> +  for (auto lib : sLibs) {

const auto&

::: dom/media/webaudio/DelayBuffer.cpp:147
(Diff revision 1)
>      double interpolationFactor = currentDelay - floorDelay;
>      int positions[2];
>      positions[1] = PositionForDelay(floorDelay) + i;
>      positions[0] = positions[1] - 1;
>  
> -    for (unsigned tick = 0; tick < ArrayLength(positions); ++tick) {
> +    for (int position : positions) {

keep it unsigned, positions above should be an unsigned too

::: dom/media/webaudio/DelayBuffer.cpp:177
(Diff revision 1)
>    const bool firstTime = mCurrentDelay < 0.0;
>    double currentDelay = firstTime ? aDelayTicks : mCurrentDelay;
>  
>    double computedDelay[WEBAUDIO_BLOCK_SIZE];
>  
> -  for (unsigned i = 0; i < WEBAUDIO_BLOCK_SIZE; ++i) {
> +  for (double & i : computedDelay) {

double&

I'm not sure C++11 loops here helps to clarity. somehow I like the previous ones better.

We're not really iterating over the array, just writing to each element.
it's clear you're writing to the array.

seems weird to me to not read the value with C++11 iterator

::: dom/media/webaudio/ScriptProcessorNode.cpp:47
(Diff revision 1)
>      size_t SizeOfExcludingThis(MallocSizeOf aMallocSizeOf) const
>      {
>        mMutex.AssertCurrentThreadOwns();
>  
>        size_t amount = 0;
> -      for (size_t i = 0; i < mBufferList.size(); i++) {
> +      for (const auto & buff : mBufferList) {

const auto&

use buffer as name

::: dom/media/webrtc/MediaTrackConstraints.h:370
(Diff revision 1)
>        aDevices.AppendElement(ordinal.second);
>      }
>  
>      // Then apply advanced constraints.
>  
> -    for (int i = 0; i < int(c.mAdvanced.size()); i++) {
> +    for (const auto & constraint : c.mAdvanced) {

const auto&

::: dom/media/webspeech/synth/speechd/SpeechDispatcherService.cpp:359
(Diff revision 1)
>      // introduced in release 0.8.2 in order to check for ABI compatibility.
>      NS_WARNING("Unsupported version of speechd detected");
>      return;
>    }
>  
> -  for (uint32_t i = 0; i < ArrayLength(kSpeechDispatcherSymbols); i++) {
> +  for (auto kSpeechDispatcherSymbol : kSpeechDispatcherSymbols) {

don't use a name start with a k ; it indicates a static constant.

speechDispatcherSymbol

Here it's important to use auto& otherwise that code will now be broken; you're no longer writing to the original array

::: dom/media/webspeech/synth/test/nsFakeSynthServices.cpp:271
(Diff revision 1)
>      nsCOMPtr<nsISpeechTask> mTask;
>      nsString mText;
>    };
>  
>    uint32_t flags = 0;
> -  for (uint32_t i = 0; i < ArrayLength(sIndirectVoices); i++) {
> +  for (const auto & sIndirectVoice : sIndirectVoices) {

const auto&

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8893333 [details]
Bug 1385571 - Convert loops to use the range-based loops (C++11)

https://reviewboard.mozilla.org/r/164424/#review169696

Actually, it isn't broken (but I agree that it is ugly).

As example:

    const int kSize=4;
    int union_map[kSize];

    for (int & j : union_map)
        j = 32+j;
    for (int j = 0; j < kSize; j++)
        std::cout << union_map[j] << std::endl;

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8893335 [details]
Bug 1385571 - Convert loops to use the range-based loops (C++11)

https://reviewboard.mozilla.org/r/164428/#review169708

::: editor/libeditor/CompositionTransaction.cpp:216
(Diff revision 1)
>  
>    nsCOMPtr<nsISelectionController> selCon;
>    aEditorBase.GetSelectionController(getter_AddRefs(selCon));
>    NS_ENSURE_TRUE(selCon, NS_ERROR_NOT_INITIALIZED);
>  
> -  for (uint32_t i = 0; i < ArrayLength(kIMESelections); ++i) {
> +  for (short kIMESelection : kIMESelections) {

nit: k prefix should be used only for const. So, make it |const short|.

::: editor/libeditor/EditorCommands.cpp:879
(Diff revision 1)
>    nsresult rv = editor->GetSelectionController(getter_AddRefs(selCont));
>    NS_ENSURE_SUCCESS(rv, rv);
>    NS_ENSURE_TRUE(selCont, NS_ERROR_FAILURE);
>  
>    // scroll commands
> -  for (size_t i = 0; i < mozilla::ArrayLength(scrollCommands); i++) {
> +  for (const auto & cmd : scrollCommands) {

make it |const auto& cmd| (i.e., remove space before &).

::: editor/libeditor/EditorCommands.cpp:888
(Diff revision 1)
>        return (selCont->*(cmd.scroll))(true);
>      }
>    }
>  
>    // caret movement/selection commands
> -  for (size_t i = 0; i < mozilla::ArrayLength(moveCommands); i++) {
> +  for (const auto & cmd : moveCommands) {

ditto.

::: editor/libeditor/EditorCommands.cpp:901
(Diff revision 1)
>        return (selCont->*(cmd.move))(true, true);
>      }
>    }
>  
>    // physical-direction movement/selection
> -  for (size_t i = 0; i < mozilla::ArrayLength(physicalCommands); i++) {
> +  for (const auto & cmd : physicalCommands) {

ditto.

::: editor/libeditor/HTMLEditRules.cpp:7364
(Diff revision 1)
>  
>  void
>  HTMLEditRules::ClearCachedStyles()
>  {
>    // clear the mPresent bits in mCachedStyles array
> -  for (size_t j = 0; j < SIZE_STYLE_TABLE; j++) {
> +  for (auto & mCachedStyle : mCachedStyles) {

ditto.

Additionally, don't use m prefix for local variable. Rename it to cachedStyle.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8893328 [details]
Bug 1385571 - Convert loops to use the range-based loops (C++11)

https://reviewboard.mozilla.org/r/164414/#review169718

::: netwerk/base/Dashboard.cpp:1023
(Diff revision 1)
>  
>  
>  static void
>  GetErrorString(nsresult rv, nsAString& errorString)
>  {
> -    for (size_t i = 0; i < ArrayLength(socketTransportStatuses); ++i) {
> +    for (auto & socketTransportStatuse : socketTransportStatuses) {

nit: socketTransportStatuse -> status

::: netwerk/cache2/CacheIOThread.cpp:626
(Diff revision 1)
>  {
>    MonitorAutoLock lock(const_cast<CacheIOThread*>(this)->mMonitor);
>  
>    size_t n = 0;
>    n += mallocSizeOf(mThread);
> -  for (uint32_t level = 0; level < LAST_LEVEL; ++level) {
> +  for (const auto & level : mEventQueue) {

nit: rename level to queue

::: netwerk/streamconv/converters/nsFTPDirListingConv.cpp:279
(Diff revision 1)
>  
>          if (type != 'd')
>          {
> -            for (int i = 0; i < int(sizeof(result.fe_size)); ++i)
> +            for (char & i : result.fe_size)
>              {
> -                if (result.fe_size[i] != '\0')
> +                if (i != '\0')

nit: rename i to c, or something else to indicate it's a reference to a char.

::: netwerk/wifi/nsWifiScannerDBus.cpp:288
(Diff revision 1)
>    }
>  
>    const uint32_t MAC_LEN = 6;
>    uint8_t macAddress[MAC_LEN];
>    char* token = strtok(hwAddress, ":");
> -  for (uint32_t i = 0; i < ArrayLength(macAddress); i++) {
> +  for (unsigned char & macAddres : macAddress) {

nit: rename macAddres to macByte

Comment 20

[Nicolas Silva [:nical]]

Comment on attachment 8893329 [details]
Bug 1385571 - Convert loops to use the range-based loops (C++11)

https://reviewboard.mozilla.org/r/164416/#review169722

A lot of it looks like it has been automaticlly generated, picking up the m- prefix in the loop variable when iterating over a collection that is a member. Please update the loop variables so that they don't have this m- prefix which is reserved to member variables (otherwise it's confusing). I started pointing them out one by one but halfway through them I figured I'd be flooding bugzilla with the same comment repeated over and over so I stopped there (but there's plenty more to fix up).

::: gfx/2d/DrawTargetTiled.cpp:165
(Diff revision 1)
>  }
>  
>  void
>  DrawTargetTiled::PopClip()
>  {
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile (mTile being expected to be a class member).

::: gfx/2d/DrawTargetTiled.cpp:184
(Diff revision 1)
>  void
>  DrawTargetTiled::CopySurface(SourceSurface *aSurface,
>                               const IntRect &aSourceRect,
>                               const IntPoint &aDestination)
>  {
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:198
(Diff revision 1)
>  }
>  
>  void
>  DrawTargetTiled::SetTransform(const Matrix& aTransform)
>  {
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:210
(Diff revision 1)
>  
>  void
>  DrawTargetTiled::SetPermitSubpixelAA(bool aPermitSubpixelAA)
>  {
>    DrawTarget::SetPermitSubpixelAA(aPermitSubpixelAA);
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:234
(Diff revision 1)
>  
>  void
>  DrawTargetTiled::FillRect(const Rect& aRect, const Pattern& aPattern, const DrawOptions& aDrawOptions)
>  {
>    Rect deviceRect = mTransform.TransformBounds(aRect);
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:251
(Diff revision 1)
>  DrawTargetTiled::Stroke(const Path* aPath, const Pattern& aPattern, const StrokeOptions& aStrokeOptions, const DrawOptions& aDrawOptions)
>  {
>    // Approximate the stroke extents, since Path::GetStrokeExtents can be slow
>    Rect deviceRect = aPath->GetBounds(mTransform);
>    deviceRect.Inflate(MaxStrokeExtents(aStrokeOptions, mTransform));
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:276
(Diff revision 1)
>      // If rects are mapped to rects, we can compute the inner rect
>      // of the stroked rect.
>      innerRect = deviceRect;
>      innerRect.Deflate(strokeMargin);
>    }
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:296
(Diff revision 1)
>  DrawTargetTiled::StrokeLine(const Point& aStart, const Point& aEnd, const Pattern& aPattern, const StrokeOptions &aStrokeOptions, const DrawOptions& aDrawOptions)
>  {
>    Rect lineBounds = Rect(aStart, Size()).UnionEdges(Rect(aEnd, Size()));
>    Rect deviceRect = mTransform.TransformBounds(lineBounds);
>    deviceRect.Inflate(MaxStrokeExtents(aStrokeOptions, mTransform));
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:311
(Diff revision 1)
>  
>  void
>  DrawTargetTiled::Fill(const Path* aPath, const Pattern& aPattern, const DrawOptions& aDrawOptions)
>  {
>    Rect deviceRect = aPath->GetBounds(mTransform);
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:329
(Diff revision 1)
>                             const Matrix& aMaskTransform, const IntRect& aBounds,
>                             bool aCopyBackground)
>  {
>    // XXX - not sure this is what we want or whether we want to continue drawing to a larger
>    // intermediate surface, that would require tweaking the code in here a little though.
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/DrawTargetTiled.cpp:347
(Diff revision 1)
>  void
>  DrawTargetTiled::PopLayer()
>  {
>    // XXX - not sure this is what we want or whether we want to continue drawing to a larger
>    // intermediate surface, that would require tweaking the code in here a little though.
> -  for (size_t i = 0; i < mTiles.size(); i++) {
> +  for (auto & mTile : mTiles) {

Please rename the loop variable mTile into tile.

::: gfx/2d/FilterNodeSoftware.cpp:857
(Diff revision 1)
>  void
>  FilterNodeSoftware::Invalidate()
>  {
>    mCachedOutput = nullptr;
>    mCachedRect = IntRect();
> -  for (std::vector<FilterInvalidationListener*>::iterator it = mInvalidationListeners.begin();
> +  for (auto & mInvalidationListener : mInvalidationListeners) {

Please rename the loop variable mInvalidationListener into listener (or something without the m).

::: gfx/2d/FilterNodeSoftware.cpp:867
(Diff revision 1)
>  FilterNodeSoftware::~FilterNodeSoftware()
>  {
>    MOZ_ASSERT(!mInvalidationListeners.size(),
>               "All invalidation listeners should have unsubscribed themselves by now!");
>  
> -  for (std::vector<RefPtr<FilterNodeSoftware> >::iterator it = mInputFilters.begin();
> +  for (auto & mInputFilter : mInputFilters) {

Please rename the loop variable mInputFilter into inputfilter (or something without the m).

::: gfx/2d/Path.cpp:154
(Diff revision 1)
>  FlattenedPath::ComputeLength()
>  {
>    if (!mCalculatedLength) {
>      Point currentPoint;
>  
> -    for (uint32_t i = 0; i < mPathOps.size(); i++) {
> +    for (auto & mPathOp : mPathOps) {

Please rename the loop variable mPathOp into pathOp (or something without the m).

::: gfx/2d/Path.cpp:177
(Diff revision 1)
>    // We track the last point that -wasn't- in the same place as the current
>    // point so if we pass the edge of the path with a bunch of zero length
>    // paths we still get the correct tangent vector.
>    Point lastPointSinceMove;
>    Point currentPoint;
> -  for (uint32_t i = 0; i < mPathOps.size(); i++) {
> +  for (auto & mPathOp : mPathOps) {

Please rename the loop variable mPathOp into pathOp (or something without the m).

::: gfx/2d/PathRecording.cpp:82
(Diff revision 1)
>    return MakeAndAddRef<PathRecording>(path, mPathOps, mFillRule);
>  }
>  
>  PathRecording::~PathRecording()
>  {
> -  for (size_t i = 0; i < mStoredRecorders.size(); i++) {
> +  for (auto & mStoredRecorder : mStoredRecorders) {

Please rename the loop variable mStoredRecorder into recorder (or something without the m).

::: gfx/2d/PathRecording.cpp:102
(Diff revision 1)
>  already_AddRefed<PathBuilder>
>  PathRecording::TransformedCopyToBuilder(const Matrix &aTransform, FillRule aFillRule) const
>  {
>    RefPtr<PathBuilder> pathBuilder = mPath->TransformedCopyToBuilder(aTransform, aFillRule);
>    RefPtr<PathBuilderRecording> recording = new PathBuilderRecording(pathBuilder, aFillRule);
> -  typedef std::vector<PathOp> pathOpVec;
> +  for (const auto & mPathOp : mPathOps) {

Please rename the loop variable mPathOp into pathOp (or something without the m).

::: gfx/2d/RecordedEventImpl.h:2317
(Diff revision 1)
>  RecordedPathCreation::Record(S &aStream) const
>  {
>    WriteElement(aStream, mRefPtr);
>    WriteElement(aStream, uint64_t(mPathOps.size()));
>    WriteElement(aStream, mFillRule);
> -  typedef std::vector<PathOp> pathOpVec;
> +  for (const auto & mPathOp : mPathOps) {

Please rename the loop variable mPathOp into something without the m.

::: gfx/2d/SFNTData.cpp:177
(Diff revision 1)
>    return hash << 32 | aDataLength;;
>  }
>  
>  SFNTData::~SFNTData()
>  {
> -  for (size_t i = 0; i < mFonts.length(); ++i) {
> +  for (auto & mFont : mFonts) {

Please rename the loop variable mFont into font or something without the m.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(I wonder why we want this. Need to be careful with ranged-based loops. They have caused several security critical crashes in cases the data structure was modified during looping. )

Comment 22

[Sylvestre Ledru [:Sylvestre]]

(In reply to Olli Pettay [:smaug] from comment #21)
> (I wonder why we want this. 
In most of the cases, the code is easier to read.

> Need to be careful with ranged-based loops. They
> have caused several security critical crashes in cases the data structure
> was modified during looping. )
Do you have some examples? I wasn't aware of this.

Comment 23

[Jean-Yves Avenard [:jya]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> (In reply to Olli Pettay [:smaug] from comment #21)
> > (I wonder why we want this. 
> In most of the cases, the code is easier to read.
> 
> > Need to be careful with ranged-based loops. They
> > have caused several security critical crashes in cases the data structure
> > was modified during looping. )
> Do you have some examples? I wasn't aware of this.

I guess range based loops are often made using iterators, and the operator++ operand on it.

Like all iterators, the may become invalid if the container is modified (like an object is removed).

AFAIK, none of your changes here encounter this case.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Note that it isn't the first batch of this change: bug 1337358 was the first (plus others bugs) and they didn't cause any regressions afaik

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Have been distracted by other things. closing.