Closed Bug 1337358 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set
normal

Tracking

(firefox54 fixed)

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(9 files)

59 bytes, text/x-review-board-request
jesup
: review+
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
bbouvier
: review+
Details
59 bytes, text/x-review-board-request
franziskus
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
froydnj
: review+
Details
59 bytes, text/x-review-board-request
jmaher
: review+
Details
59 bytes, text/x-review-board-request
keeler
: review+
Details
59 bytes, text/x-review-board-request
gcp
: review+
Details
Running again clang tidy on the whole code to update loops
Comment on attachment 8834384 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in js/

https://reviewboard.mozilla.org/r/110330/#review111518

OK, with nits addressed. Thanks!

::: js/src/ctypes/CTypes.cpp:4573
(Diff revision 1)
>      MOZ_ASSERT(fninfo);
>  
>      // Identify our objects to the tracer.
>      JS::TraceEdge(trc, &fninfo->mABI, "abi");
>      JS::TraceEdge(trc, &fninfo->mReturnType, "returnType");
> -    for (size_t i = 0; i < fninfo->mArgTypes.length(); ++i)
> +    for (auto & mArgType : fninfo->mArgTypes)

nit: local style is `auto& mArgType`

::: js/src/frontend/Parser.cpp:941
(Diff revision 1)
>      MOZ_ASSERT(pc->isFunctionBox() && pc->functionBox()->hasSimpleParameterList());
>  
>      if (pc->functionBox()->hasDuplicateParameters)
>          return false;
>  
> -    for (size_t i = 0; i < pc->positionalFormalParameterNames().length(); i++) {
> +    for (auto name : pc->positionalFormalParameterNames()) {

nit: `auto* name`

::: js/src/jsarray.cpp:2868
(Diff revision 1)
>  
>      if (!success)
>          return SliceSlowly(cx, obj, begin, end, result);
>  
>      RootedValue value(cx);
> -    for (size_t i = 0, len = indexes.length(); i < len; i++) {
> +    for (unsigned int index : indexes) {

nit: size_t
Attachment #8834384 - Flags: review?(bbouvier) → review+
Comment on attachment 8834386 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in modules/libjar/

https://reviewboard.mozilla.org/r/110334/#review111522

Thanks.  r=me with the below changes.

::: modules/libjar/nsZipArchive.cpp:410
(Diff revision 1)
>        return NS_OK;
>      return ExtractFile(currItem, 0, 0);
>    }
>  
>    // test all items in archive
> -  for (int i = 0; i < ZIP_TABSIZE; i++) {
> +  for (auto & mFile : mFiles) {

Please don't call this `mFile`, since that would indicate that you're storing it into a member variable, which is not the case.  Perhaps `auto* item`?

::: modules/libjar/nsZipArchive.cpp:805
(Diff revision 1)
>    mBuiltSynthetics = true;
>  
>  MOZ_WIN_MEM_TRY_BEGIN
>    // Create synthetic entries for any missing directories.
>    // Do this when all ziptable has scanned to prevent double entries.
> -  for (int i = 0; i < ZIP_TABSIZE; ++i)
> +  for (auto item : mFiles)

Please make this `auto* item`, so it's more obvious that it's a pointer and to insulate us from any non-obvious behavior (e.g. copy-constructing a smart pointer).
Attachment #8834386 - Flags: review?(nfroyd) → review+
Comment on attachment 8834387 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in tools/

https://reviewboard.mozilla.org/r/110336/#review111528

Thanks.  r=me with the below change.

::: tools/profiler/core/platform-linux.cc:302
(Diff revision 1)
>  
>      if (!gSampler->IsPaused()) {
>        StaticMutexAutoLock lock(Sampler::sRegisteredThreadsMutex);
>  
>        bool isFirstProfiledThread = true;
> -      for (uint32_t i = 0; i < Sampler::sRegisteredThreads->size(); i++) {
> +      for (auto info : *Sampler::sRegisteredThreads) {

`auto* info`, please.
Attachment #8834387 - Flags: review?(nfroyd) → review+
Comment on attachment 8834384 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in js/

https://reviewboard.mozilla.org/r/110330/#review111518

> nit: local style is `auto& mArgType`

Actually, please just name this `argType` (drop the `m` prefix)
Comment on attachment 8834385 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in browser/

https://reviewboard.mozilla.org/r/110332/#review111540
Attachment #8834385 - Flags: review?(franziskuskiefer) → review+
Comment on attachment 8834388 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in testing/mochitest/

https://reviewboard.mozilla.org/r/110338/#review111594

::: testing/mochitest/ssltunnel/ssltunnel.cpp:1031
(Diff revision 1)
>    return PL_strdup("");
>  }
>  
>  server_info_t* findServerInfo(int portnumber)
>  {
> -  for (vector<server_info_t>::iterator it = servers.begin();
> +  for (auto & server : servers) 

nit: trailing whitespace here.

::: testing/mochitest/ssltunnel/ssltunnel.cpp:1612
(Diff revision 1)
>    PR_DestroyLock(shutdown_lock);
>    if (NSS_Shutdown() == SECFailure) {
>      LOG_DEBUG(("Leaked NSS objects!\n"));
>    }
>    
> -  for (vector<server_info_t>::iterator it = servers.begin();
> +  for (auto & server : servers) 

nit: trailing whitepace here
Attachment #8834388 - Flags: review?(jmaher) → review+
Attachment #8834382 - Flags: review?(rjesup)
Attachment #8834383 - Flags: review?(rjesup)
Comment on attachment 8834389 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/

https://reviewboard.mozilla.org/r/110340/#review111702

r=me for the security/pkix changes with comments addressed

::: security/pkix/lib/pkixnames.cpp:1611
(Diff revision 1)
>  bool
>  StartsWithIDNALabel(Input id)
>  {
>    static const uint8_t IDN_ALABEL_PREFIX[4] = { 'x', 'n', '-', '-' };
>    Reader input(id);
> -  for (size_t i = 0; i < sizeof(IDN_ALABEL_PREFIX); ++i) {
> +  for (unsigned char prefix : IDN_ALABEL_PREFIX) {

I would prefer `const uint8_t prefixByte`

::: security/pkix/lib/pkixocsp.cpp:967
(Diff revision 1)
>    *d++ = 0x30; *d++ = totalLen - 6u;  //     requestList (SEQUENCE OF)
>    *d++ = 0x30; *d++ = totalLen - 8u;  //       Request (SEQUENCE)
>    *d++ = 0x30; *d++ = totalLen - 10u; //         reqCert (CertID SEQUENCE)
>  
>    // reqCert.hashAlgorithm
> -  for (size_t i = 0; i < sizeof(hashAlgorithm); ++i) {
> +  for (unsigned char i : hashAlgorithm) {

`const uint8_t hashAlgorithmByte`

::: security/sandbox/linux/gtest/TestBroker.cpp:101
(Diff revision 1)
>      mClient.reset(new SandboxBrokerClient(rawFD.release()));
>    }
>  
>    template<class C, void (C::* Main)()>
>    void StartThread(pthread_t *aThread) {
>      ASSERT_EQ(0, pthread_create(aThread, nullptr, ThreadMain<C, Main>,

I'm not a sandboxing peer - someone from this list should review: https://wiki.mozilla.org/Modules/All#Sandboxing
Attachment #8834389 - Flags: review?(dkeeler) → review+
Comment on attachment 8834382 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/webrtc/signaling/ =jesup

https://reviewboard.mozilla.org/r/110326/#review111706

r+ with nits:
mFoo -> foo
sFoo -> foo
"auto & foo" -> "auto& foo"
And one question about why there wasn't an &

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1814
(Diff revision 1)
>  
>  nsresult
>  JsepSessionImpl::SetRemoteTracksFromDescription(const Sdp* remoteDescription)
>  {
>    // Unassign all remote tracks
> -  for (auto i = mRemoteTracks.begin(); i != mRemoteTracks.end(); ++i) {
> +  for (auto & mRemoteTrack : mRemoteTracks) {

IIRC it's usually coded as "auto& foo" not "auto & foo" -- 412 auto& foo, 6 auto & foo (and 81 auto &foo).

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:1140
(Diff revision 1)
>  }
>  
>  void
>  WebrtcAudioConduit::DumpCodecDB() const
>   {
> -    for(std::vector<AudioCodecConfig*>::size_type i=0;i < mRecvCodecList.size();i++)
> +    for(auto codec : mRecvCodecList)

was it intentional to not use & here?

::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp:267
(Diff revision 1)
>    }
>    dtls->SetIdentity(pcid);
>  
>    const SdpFingerprintAttributeList& fingerprints =
>        aTransport.mDtls->GetFingerprints();
> -  for (auto fp = fingerprints.mFingerprints.begin();
> +  for (const auto & mFingerprint : fingerprints.mFingerprints) {

I'd prefer to see local vars not named mFoo; when reading code later on one would assume that mFoo was a member var, and it isn't.  Just call it 'foo':
 for (const auto& fingerprint : fingerprints.mFingerprints) {

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1914
(Diff revision 1)
>      }
>  
>      tracksByStreamId[track->GetStreamId()].push_back(track);
>    }
>  
> -  for (auto i = tracksByStreamId.begin(); i != tracksByStreamId.end(); ++i) {
> +  for (auto & sId : tracksByStreamId) {

don't name a local var sFoo - that implies a static.   Use 'id' here

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:176
(Diff revision 1)
>  void SourceStreamInfo::DetachTransport_s()
>  {
>    ASSERT_ON_THREAD(mParent->GetSTSThread());
>    // walk through all the MediaPipelines and call the shutdown
>    // transport functions. Must be on the STS thread.
> -  for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) {
> +  for (auto & mPipeline : mPipelines) {

ditto mFoo

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:187
(Diff revision 1)
>  {
>    ASSERT_ON_THREAD(mParent->GetMainThread());
>  
>    // walk through all the MediaPipelines and call the shutdown
>    // media functions. Must be on the main thread.
> -  for (auto it = mPipelines.begin(); it != mPipelines.end(); ++it) {
> +  for (auto & mPipeline : mPipelines) {

ditto mFoo

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:742
(Diff revision 1)
>  PeerConnectionMedia::FinalizeIceRestart_s()
>  {
>    ASSERT_ON_THREAD(mSTSThread);
>  
>    // reset old streams since we don't need them anymore
> -  for (auto i = mTransportFlows.begin();
> +  for (auto & mTransportFlow : mTransportFlows) {

ditto mFoo
Attachment #8834382 - Flags: review?(rjesup) → review+
Comment on attachment 8834383 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ =jesup

https://reviewboard.mozilla.org/r/110328/#review111718

revectoring review to bwc
I'd apply the nit issues I mentioned in my other review (mFoo -> foo, "auto & foo" -> "auto& foo"
Attachment #8834383 - Flags: review?(rjesup)
Comment on attachment 8834382 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/webrtc/signaling/ =jesup

https://reviewboard.mozilla.org/r/110326/#review111952

::: media/webrtc/signaling/src/media-conduit/AudioConduit.cpp:1140
(Diff revision 1)
>  }
>  
>  void
>  WebrtcAudioConduit::DumpCodecDB() const
>   {
> -    for(std::vector<AudioCodecConfig*>::size_type i=0;i < mRecvCodecList.size();i++)
> +    for(auto codec : mRecvCodecList)

Nope, fixed
Attachment #8834883 - Flags: review?(jmathies)
Comment on attachment 8834383 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ =jesup

Grr, mozreview took my change but didn't propagate it
Attachment #8834383 - Flags: review?(rjesup) → review?(docfaraday)
Comment on attachment 8834383 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ =jesup

https://reviewboard.mozilla.org/r/110328/#review112442

::: media/mtransport/test/ice_unittest.cpp:620
(Diff revision 2)
> -    for (size_t i=0; i < candidates_in.size(); i++) {
> -      std::string candidate(FilterCandidate(candidates_in[i]));
> +    for (const auto& i : candidates_in) {
> +      std::string candidate(FilterCandidate(i));

I guess call this something other than |i|?

::: media/mtransport/test/ice_unittest.cpp:984
(Diff revision 2)
>  
>    void Shutdown() {
>      std::cerr << name_ << " Shutdown" << std::endl;
>      shutting_down_ = true;
> -    for (auto s = controlled_trickle_candidates_.begin();
> -         s != controlled_trickle_candidates_.end();
> +    for (auto& controlled_trickle_candidate : controlled_trickle_candidates_) {
> +      for (auto cand = controlled_trickle_candidate.second.begin(); cand != controlled_trickle_candidate.second.end(); ++cand) {

We can simplify this loop too.
Attachment #8834383 - Flags: review?(docfaraday) → review+
Attachment #8834883 - Flags: review?(jmathies)
Comment on attachment 8834383 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ =jesup

https://reviewboard.mozilla.org/r/110328/#review112972

::: media/mtransport/test/ice_unittest.cpp:620
(Diff revisions 2 - 3)
> -    for (const auto& i : candidates_in) {
> -      std::string candidate(FilterCandidate(i));
> +    for (const auto& candidate_ : candidates_in) {
> +      std::string candidate(FilterCandidate(candidate_));

trailing underscores are a pattern in mtransport (and much google-formatted code) for a member variable (equivalent to mFoo in mozilla code), so this add confusion.  Perhaps a_candidate?
Attachment #8834383 - Flags: review?(rjesup)
Attachment #8834883 - Flags: review?(jmathies)
Comment on attachment 8834383 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ =jesup

https://reviewboard.mozilla.org/r/110328/#review113110

LGTM.   Byron?
Attachment #8834383 - Flags: review?(rjesup)
Comment on attachment 8834883 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/sandbox/

https://reviewboard.mozilla.org/r/110682/#review113290

::: security/sandbox/linux/gtest/TestBroker.cpp:109
(Diff revision 3)
>  
>    template<class C, void (C::* Main)()>
>    void RunOnManyThreads() {
>      static const int kNumThreads = 5;
>      pthread_t threads[kNumThreads];
> -    for (int i = 0; i < kNumThreads; ++i) {
> +    for (unsigned long & thread : threads) {

Should be pthread_t or auto.

::: security/sandbox/linux/gtest/TestBroker.cpp:112
(Diff revision 3)
>      static const int kNumThreads = 5;
>      pthread_t threads[kNumThreads];
> -    for (int i = 0; i < kNumThreads; ++i) {
> -      StartThread<C, Main>(&threads[i]);
> +    for (unsigned long & thread : threads) {
> +      StartThread<C, Main>(&thread);
>      }
> -    for (int i = 0; i < kNumThreads; ++i) {
> +    for (unsigned long thread : threads) {

Same.
Attachment #8834883 - Flags: review?(gpascutto)
Comment on attachment 8834883 [details]
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/sandbox/

https://reviewboard.mozilla.org/r/110682/#review113292
Attachment #8834883 - Flags: review?(gpascutto) → review+
Pushed by sledru@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6be51ae5d931
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/webrtc/signaling/ r=jesup=jesup
https://hg.mozilla.org/integration/autoland/rev/dcd3b1f4bc69
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ r=bwc=jesup
https://hg.mozilla.org/integration/autoland/rev/7bdff34f5eab
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in js/ r=bbouvier
https://hg.mozilla.org/integration/autoland/rev/7e5f3ebcd77c
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in browser/ r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/50e0a1c9d2ce
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in modules/libjar/ r=froydnj
https://hg.mozilla.org/integration/autoland/rev/5fc40ff28746
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in tools/ r=froydnj
https://hg.mozilla.org/integration/autoland/rev/4d65d08fe6c0
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in testing/mochitest/ r=jmaher
https://hg.mozilla.org/integration/autoland/rev/a39e29e8f864
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/ r=keeler
https://hg.mozilla.org/integration/autoland/rev/2f804e38c8cb
Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/sandbox/ r=gcp
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1b280faca1c227646818c3f32e43120683977f
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in js/ - follow up r=bbouvier
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.