Bug 129990 – Avoid copy-prone idiom "for (auto item : collection)"

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

[Darin Adler]

Avoid copy-prone idiom "for (auto item : collection)"

[Darin Adler]

Created attachment 226253 [details]
Patch

[Darin Adler]

Created attachment 226317 [details]
Patch

[Darin Adler]

To find these I used this command:

% git grep "for (auto .*:"

[Alexey Proskuryakov]

Comment on attachment 226317 [details]
Patch

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

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:344
> -    for (auto breakpointID : breakpointIDs) {
> +    for (auto& breakpointID : m_breakpointIdentifierToDebugServerBreakpointIDs.take(breakpointIdentifier)) {

breakpointID is a size_t, so auto& isn't making the code better.

I'd much prefer an explicitly named type to auto here.

[Darin Adler]

(In reply to comment #4)
> I'd much prefer an explicitly named type to auto here.

I’ll change it. But don’t you always prefer an explicitly named type?

[Darin Adler]

(In reply to comment #4)
> > Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:344
> > -    for (auto breakpointID : breakpointIDs) {
> > +    for (auto& breakpointID : m_breakpointIdentifierToDebugServerBreakpointIDs.take(breakpointIdentifier)) {
> 
> breakpointID is a size_t, so auto& isn't making the code better.

Right, but it also doesn’t make it any worse.

[Darin Adler]

Created attachment 226468 [details]
Patch

[Geoffrey Garen]

Comment on attachment 226468 [details]
Patch

r=me

[Darin Adler]

Committed r165546: <http://trac.webkit.org/changeset/165546>

[Antti Koivisto]

Comment on attachment 226468 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:730
> -    HashSet<LiveNodeList*> liveNodeLists = std::move(m_listsInvalidatedAtDocument);
> -    for (auto it : liveNodeLists)
> -        it->invalidateCache(attrName);
> -
> -    HashSet<HTMLCollection*> collectionLists = std::move(m_collectionsInvalidatedAtDocument);
> -    for (auto it : collectionLists)
> -        it->invalidateCache(attrName);
> +    for (auto* list : std::move(m_listsInvalidatedAtDocument))
> +        list->invalidateCache(attrName);
> +    for (auto* list : std::move(m_collectionsInvalidatedAtDocument))
> +        list->invalidateCache(attrName);

Looks like this part is making many tests crash. I suppose the collection dies immediately after the move.

[Antti Koivisto]

Reverted that part in https://trac.webkit.org/r165566

[Jer Noble]

This seems to have caused 50+ tests to assert:

<http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r165564%20(3310)/results.html>

[Jer Noble]

Oh, thanks Antti. :)

[Darin Adler]

(In reply to comment #11)
> Reverted that part in https://trac.webkit.org/r165566

Thanks Antti. I would have used auto in the revert, but otherwise it’s exactly what I would do.

[Ryosuke Niwa]

Comment on attachment 226468 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:730
>> +        list->invalidateCache(attrName);
> 
> Looks like this part is making many tests crash. I suppose the collection dies immediately after the move.

Yeah, the whole point of this code was to swap.

[Darin Adler]

(In reply to comment #15)
> Yeah, the whole point of this code was to swap.

No, the point of the code was to move. The swap is just a tricky way to do a move when C++ didn’t have swap.

[Darin Adler]

when C++ didn’t have move.

Antti’s patch works around some problem related to doing a move on the collection in a for loop.

[Ryosuke Niwa]

Comment on attachment 226468 [details]
Patch

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

>>> Source/WebCore/dom/Node.cpp:730
>>> +        list->invalidateCache(attrName);
>> 
>> Looks like this part is making many tests crash. I suppose the collection dies immediately after the move.
> 
> Yeah, the whole point of this code was to swap.

I had a chance to talk with someone work who works on clang,
and he clarified to me that std::move doesn't create a copy.
It simply casts & to &&, so the temporary is never created here.