70285 2011-10-17 16:43:06 -0700 Style guide should mention the preference of index over iterator 2011-10-21 16:17:55 -0700 1 1 1 Unclassified WebKit WebKit Website 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 rniwa rniwa adamk darin sam oldest_to_newest 485627 0 rniwa 2011-10-17 16:43:06 -0700 Per discussion on webkit-dev, we should mention that we prefer using index over iterator to go though items in a Vector. 485642 1 rniwa 2011-10-17 16:59:05 -0700 https://lists.webkit.org/pipermail/webkit-dev/2011-October/018274.html 485643 2 111351 rniwa 2011-10-17 17:01:11 -0700 Created attachment 111351 70285 486324 3 rniwa 2011-10-18 14:50:39 -0700 Any reviewer? 486331 4 111351 darin 2011-10-18 15:02:03 -0700 Comment on attachment 111351 70285 For the index case we chose to evaluate size every time through the loop. For the iterator case we chose to cache the end iterator. That’s an arbitrary difference not related to index vs. iterator. I think a local variable named size would be a better way to do things. I don’t think this makes it clear enough that there are data structures where iterators are present, but indexing is not. Nor does it answer the question of why we offer iterators if indexing are preferred. Also, this doesn’t contain any of the rationale; perhaps it should. 487093 5 111690 rniwa 2011-10-19 16:06:19 -0700 Created attachment 111690 Updated per comment 488573 6 rniwa 2011-10-21 12:50:46 -0700 Ping reviewers again 488681 7 111690 darin 2011-10-21 14:57:44 -0700 Comment on attachment 111690 Updated per comment View in context: https://bugs.webkit.org/attachment.cgi?id=111690&action=review > Websites/webkit.org/coding/coding-style.html:712 > +<li>Prefer index over iterator in Vector iterations for a terse, easier-to-read code, and to avoid accessing freed memory I don’t see the end of this <li> element. It’s overselling this technique to say that it avoids accessing freed memory, because using a too high index can still access freed memory. > Websites/webkit.org/coding/coding-style.html:717 > +size_t frameViewSize = frameViews.size(); frameViewSize is not the right name here. Maybe frameViewsSize or frameViewsCount or size or count. 488708 8 rniwa 2011-10-21 15:10:06 -0700 (In reply to comment #7) > (From update of attachment 111690 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111690&action=review > > > Websites/webkit.org/coding/coding-style.html:712 > > +<li>Prefer index over iterator in Vector iterations for a terse, easier-to-read code, and to avoid accessing freed memory > > I don’t see the end of this <li> element. It's the pattern used throughout the file. I should probably correct that once for all. > It’s overselling this technique to say that it avoids accessing freed memory, because using a too high index can still access freed memory. Okay, I'll remove that. > > Websites/webkit.org/coding/coding-style.html:717 > > +size_t frameViewSize = frameViews.size(); > > frameViewSize is not the right name here. Maybe frameViewsSize or frameViewsCount or size or count. Oops, typo. It was supposed to be frameViewsSize but I guess frameViewsCount sounds better. Will change before landing. 488752 9 rniwa 2011-10-21 16:17:55 -0700 Committed r98166: <http://trac.webkit.org/changeset/98166> 111351 2011-10-17 17:01:11 -0700 2011-10-19 16:06:14 -0700 70285 bug-70285-20111017170110.patch text/plain 1640 rniwa SW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNp dGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCShyZXZpc2lvbiA5NzY3NikKKysrIFdlYnNpdGVzL3dl YmtpdC5vcmcvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEt MTAtMTcgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgU3R5bGUg Z3VpZGUgc2hvdWxkIG1lbnRpb24gdGhlIHByZWZlcmVuY2Ugb2YgaW5kZXggb3ZlciBpdGVyYXRv cgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzAyODUK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQZXIgZGlz Y3Vzc2lvbiBvbiBodHRwczovL2xpc3RzLndlYmtpdC5vcmcvcGlwZXJtYWlsL3dlYmtpdC1kZXYv MjAxMS1PY3RvYmVyLzAxODI3NC5odG1sLAorICAgICAgICB3ZSBwcmVmZXIgaW5kZXggb3ZlciBp dGVyYXRvcnMuCisKKyAgICAgICAgKiBjb2RpbmcvY29kaW5nLXN0eWxlLmh0bWw6CisKIDIwMTEt MTAtMTAgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgRml4IGEg dHlwbyBwb2ludGVkIGJ5IFNhbSAoV2VpbmlnKS4KSW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcv Y29kaW5nL2NvZGluZy1zdHlsZS5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNpdGVzL3dlYmtpdC5v cmcvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCShyZXZpc2lvbiA5NzYzNSkKKysrIFdlYnNpdGVz L3dlYmtpdC5vcmcvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC03 MDgsNiArNzA4LDIxIEBAIE15Q2xhc3M6Ok15Q2xhc3MoRG9jdW1lbnQqIGRvYykgOiBNeVN1cGUK IAogTXlPdGhlckNsYXNzOjpNeU90aGVyQ2xhc3MoKSA6IE15U3VwZXJDbGFzcygpIHt9CiA8L3By ZT4KKworPGxpPlByZWZlciBpbmRleCBvdmVyIGl0ZXJhdG9yIGluIGxvb3BzLgorCis8aDQgY2xh c3M9InJpZ2h0Ij5SaWdodDo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+Citmb3IgKHNpemVfdCBp ID0gaTsgaSAmbHQ7IGZyYW1lVmlld3Muc2l6ZSgpOyArK2kpCisgICAgZnJhbWVWaWV3c1tpXS0+ dXBkYXRlTGF5b3V0QW5kU3R5bGVJZk5lZWRlZFJlY3Vyc2l2ZSgpOworPC9wcmU+CisKKzxoNCBj bGFzcz0id3JvbmciPldyb25nOjwvaDQ+Cis8cHJlIGNsYXNzPSJjb2RlIj4KK2NvbnN0IFZlY3Rv ciZsdDtSZWZQdHImbHQ7RnJhbWVWaWV3Jmd0OyAmZ3Q7OjppdGVyYXRvciBlbmQgPSBmcmFtZVZp ZXdzLmVuZCgpOworZm9yIChWZWN0b3ImbHQ7UmVmUHRyJmx0O0ZyYW1lVmlldyZndDsgJmd0Ozo6 aXRlcmF0b3IgaXQgPSBmcmFtZVZpZXdzLmJlZ2luKCk7IGl0ICE9IGVuZDsgKytpdCkKKyAgICAo Kml0KS0mZ3Q7dXBkYXRlTGF5b3V0QW5kU3R5bGVJZk5lZWRlZFJlY3Vyc2l2ZSgpOworPC9wcmU+ CiA8L29sPgogCiA8aDM+UG9pbnRlcnMgYW5kIFJlZmVyZW5jZXM8L2gzPgo= 111690 2011-10-19 16:06:19 -0700 2011-10-21 14:57:44 -0700 Updated per comment bug-70285-20111019160618.patch text/plain 1851 rniwa SW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNp dGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCShyZXZpc2lvbiA5NzgzMCkKKysrIFdlYnNpdGVzL3dl YmtpdC5vcmcvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEt MTAtMTkgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CisKKyAgICAgICAgU3R5bGUg Z3VpZGUgc2hvdWxkIG1lbnRpb24gdGhlIHByZWZlcmVuY2Ugb2YgaW5kZXggb3ZlciBpdGVyYXRv cgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzAyODUK KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQZXIgZGlz Y3Vzc2lvbiBvbiBodHRwczovL2xpc3RzLndlYmtpdC5vcmcvcGlwZXJtYWlsL3dlYmtpdC1kZXYv MjAxMS1PY3RvYmVyLzAxODI3NC5odG1sLAorICAgICAgICB3ZSBwcmVmZXIgaW5kZXggb3ZlciBp dGVyYXRvcnMuCisKKyAgICAgICAgKiBjb2RpbmcvY29kaW5nLXN0eWxlLmh0bWw6CisKIDIwMTEt MTAtMTAgIFJ5b3N1a2UgTml3YSAgPHJuaXdhQHdlYmtpdC5vcmc+CiAKICAgICAgICAgRml4IGEg dHlwbyBwb2ludGVkIGJ5IFNhbSAoV2VpbmlnKS4KSW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcv Y29kaW5nL2NvZGluZy1zdHlsZS5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNpdGVzL3dlYmtpdC5v cmcvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCShyZXZpc2lvbiA5NzgzMCkKKysrIFdlYnNpdGVz L3dlYmtpdC5vcmcvY29kaW5nL2NvZGluZy1zdHlsZS5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC03 MDgsNiArNzA4LDIzIEBAIE15Q2xhc3M6Ok15Q2xhc3MoRG9jdW1lbnQqIGRvYykgOiBNeVN1cGUK IAogTXlPdGhlckNsYXNzOjpNeU90aGVyQ2xhc3MoKSA6IE15U3VwZXJDbGFzcygpIHt9CiA8L3By ZT4KKworPGxpPlByZWZlciBpbmRleCBvdmVyIGl0ZXJhdG9yIGluIFZlY3RvciBpdGVyYXRpb25z IGZvciBhIHRlcnNlLCBlYXNpZXItdG8tcmVhZCBjb2RlLCBhbmQgdG8gYXZvaWQgYWNjZXNzaW5n IGZyZWVkIG1lbW9yeQord2hlbiBlbGVtZW50cyBhcmUgYWRkZWQgb3IgcmVtb3ZlZCBkdXJpbmcg dGhlIGl0ZXJhdGlvbiBpbnRlbnRpb25hbGx5IG9yIHVuaW50ZW50aW9uYWxseS4KKworPGg0IGNs YXNzPSJyaWdodCI+UmlnaHQ6PC9oND4KKzxwcmUgY2xhc3M9ImNvZGUiPgorc2l6ZV90IGZyYW1l Vmlld1NpemUgPSBmcmFtZVZpZXdzLnNpemUoKTsKK2ZvciAoc2l6ZV90IGkgPSBpOyBpICZsdDsg ZnJhbWVWaWV3U2l6ZTsgKytpKQorICAgIGZyYW1lVmlld3NbaV0tPnVwZGF0ZUxheW91dEFuZFN0 eWxlSWZOZWVkZWRSZWN1cnNpdmUoKTsKKzwvcHJlPgorCis8aDQgY2xhc3M9Indyb25nIj5Xcm9u Zzo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+Citjb25zdCBWZWN0b3ImbHQ7UmVmUHRyJmx0O0Zy YW1lVmlldyZndDsgJmd0Ozo6aXRlcmF0b3IgZW5kID0gZnJhbWVWaWV3cy5lbmQoKTsKK2ZvciAo VmVjdG9yJmx0O1JlZlB0ciZsdDtGcmFtZVZpZXcmZ3Q7ICZndDs6Oml0ZXJhdG9yIGl0ID0gZnJh bWVWaWV3cy5iZWdpbigpOyBpdCAhPSBlbmQ7ICsraXQpCisgICAgKCppdCktJmd0O3VwZGF0ZUxh eW91dEFuZFN0eWxlSWZOZWVkZWRSZWN1cnNpdmUoKTsKKzwvcHJlPgogPC9vbD4KIAogPGgzPlBv aW50ZXJzIGFuZCBSZWZlcmVuY2VzPC9oMz4K