Closed Bug 1337358 Opened 8 years ago Closed 8 years ago

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

Categories

(Developer Infrastructure :: Source Code Analysis, defect)

Product:
Developer Infrastructure
Component:
Source Code Analysis
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

Details

Attachments

(9 files)

Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/webrtc/signaling/ =jesup
59 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in media/mtransport/ =jesup
59 bytes, text/x-review-board-request
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in js/
59 bytes, text/x-review-board-request
bbouvier
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in browser/
59 bytes, text/x-review-board-request
franziskus
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in modules/libjar/
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in tools/
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in testing/mochitest/
59 bytes, text/x-review-board-request
jmaher
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/
59 bytes, text/x-review-board-request
keeler
: review+
Details
Bug 1337358 - Converts for(...; ...; ...) loops to use the new range-based loops in C++11 in security/sandbox/
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+
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
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: