Avoid copy-prone idiom "for (auto item : collection)"
Created attachment 226253 [details] Patch
Created attachment 226317 [details] Patch
To find these I used this command: % git grep "for (auto .*:"
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.
(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?
(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.
Created attachment 226468 [details] Patch
Comment on attachment 226468 [details] Patch r=me
Committed r165546: <http://trac.webkit.org/changeset/165546>
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.
Reverted that part in https://trac.webkit.org/r165566
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>
Oh, thanks Antti. :)
(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.
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.
(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.
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.
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.