Bug 130714 – Adopt range-based for loops to TextCheckerEnchant

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jinwoo Song]

SSIA.

[Jinwoo Song]

Created attachment 227732 [details]
Patch

[Zan Dobersek]

Comment on attachment 227732 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227732&action=review

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:57
> -    for (Vector<EnchantDict*>::const_iterator iter = m_enchantDictionaries.begin(); iter != m_enchantDictionaries.end(); ++iter)
> -        enchant_dict_add_to_session(*iter, word.utf8().data(), -1);
> +    for (auto iter : m_enchantDictionaries)
> +        enchant_dict_add_to_session(iter, word.utf8().data(), -1);

Use auto&, or even const auto& where possible in range-based for loops. This avoids unnecessary copies of the iterated objects. The benefit in this specific iteration is perhaps small since you'd be copying a pointer, but it's consistent with such loops elsewhere in the WebKit code base, and is generally a good practice.

Also, with this type of iteration 'iter' can be replaced with a more appropriate, verbose name. In this case perhaps 'dictionay' would suit better.

This applies to all the other loops that are being changed in this patch.

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:143
> -        for (Vector<String>::const_iterator iter = languages.begin(); iter != languages.end(); ++iter) {
> -            CString currentLanguage = iter->utf8();
> +        for (auto iter : languages) {
> +            CString currentLanguage = iter.utf8();

As a contrast, this copies every String object in the vector since you're not taking a reference of the objects.

[Build Bot]

Comment on attachment 227732 [details]
Patch

Attachment 227732 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4920358457049088

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html

[Build Bot]

Created attachment 227739 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Jinwoo Song]

(In reply to comment #2)
> (From update of attachment 227732 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227732&action=review
> 
> > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:57
> > -    for (Vector<EnchantDict*>::const_iterator iter = m_enchantDictionaries.begin(); iter != m_enchantDictionaries.end(); ++iter)
> > -        enchant_dict_add_to_session(*iter, word.utf8().data(), -1);
> > +    for (auto iter : m_enchantDictionaries)
> > +        enchant_dict_add_to_session(iter, word.utf8().data(), -1);
> 
> Use auto&, or even const auto& where possible in range-based for loops. This avoids unnecessary copies of the iterated objects. The benefit in this specific iteration is perhaps small since you'd be copying a pointer, but it's consistent with such loops elsewhere in the WebKit code base, and is generally a good practice.
> 
> Also, with this type of iteration 'iter' can be replaced with a more appropriate, verbose name. In this case perhaps 'dictionay' would suit better.
> 
> This applies to all the other loops that are being changed in this patch.
> 
> > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:143
> > -        for (Vector<String>::const_iterator iter = languages.begin(); iter != languages.end(); ++iter) {
> > -            CString currentLanguage = iter->utf8();
> > +        for (auto iter : languages) {
> > +            CString currentLanguage = iter.utf8();
> 
> As a contrast, this copies every String object in the vector since you're not taking a reference of the objects.

Thanks for kind advice Zan! I'll upload a new patch.

[Jinwoo Song]

Created attachment 228017 [details]
Patch

[Darin Adler]

Comment on attachment 228017 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=228017&action=review

> Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:56
> +    for (const auto& dictionary : m_enchantDictionaries)

I would have suggested auto& without the const for all of these loops. While I understand that we can say const, it adds very little.

[Jinwoo Song]

Created attachment 228136 [details]
Patch

Applied Darin's comment.

[Build Bot]

Comment on attachment 228136 [details]
Patch

Attachment 228136 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5089131008884736

New failing tests:
compositing/columns/composited-rl-paginated-repaint.html

[Build Bot]

Created attachment 228141 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Jinwoo Song]

(In reply to comment #10)
> Created an attachment (id=228141) [details]
> Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
> 
> The attached test failures were seen while running run-webkit-tests on the mac-ews.
> Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

Flaky test result.

[WebKit Commit Bot]

Comment on attachment 228136 [details]
Patch

Clearing flags on attachment: 228136

Committed r166474: <http://trac.webkit.org/changeset/166474>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.