Bug 82784 – Rename first/second to key/value in HashMap iterators

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Caio Marcelo de Oliveira Filho]

As suggested in comment https://bugs.webkit.org/show_bug.cgi?id=71063#c2 it would be better (more readable at callsites) to have 'it->key' and 'it->value' instead of 'it->first' and 'it->second'.

[Caio Marcelo de Oliveira Filho]

Created attachment 134905 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Darin Adler, could you take a look at this one? The most interesting bits are in Source/WTF, in particular HashTraits.h.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[WebKit Review Bot]

Attachment 134905 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1
Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:319:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 352 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 134905 [details]
Patch

Attachment 134905 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12266568

[Build Bot]

Comment on attachment 134905 [details]
Patch

Attachment 134905 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12253515

[Ryosuke Niwa]

The patch looks great in general but it seems like you still have to fix some builds. And I'd differ the review of traits changes to veteran WTF reviewers :)

[Caio Marcelo de Oliveira Filho]

Created attachment 135113 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Created attachment 135116 [details]
Patch

[Build Bot]

Comment on attachment 135116 [details]
Patch

Attachment 135116 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12306757

[Caio Marcelo de Oliveira Filho]

Created attachment 135122 [details]
Patch

[Eric Seidel (no email)]

Ideally maciej/darin would take a look at this.

[Maciej Stachowiak]

Comment on attachment 135122 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        HashMapEntry. In HashMap the ValueType serve as "contents" of the iterators.

I would suggest naming this KeyValuePair instead of HashMapEntry. It seems like in principle it could be useful in other contexts where you have a key/value pair and want better names than first/second.

[Caio Marcelo de Oliveira Filho]

Created attachment 138874 [details]
Patch

[Caio Marcelo de Oliveira Filho]

New rebased version of the patch. I also changed HashMapEntry to KeyValuePair, but kept it defined in HashTraits.h for now. If we decided to go with it, I can create a separate file for KeyValuePair in this or a different patch

[Caio Marcelo de Oliveira Filho]

Ping?

[Ryosuke Niwa]

(In reply to comment #16)
> Ping?

You probably need to update the patch for ToT.

[Caio Marcelo de Oliveira Filho]

Comment on attachment 138874 [details]
Patch

Removing r? until I upload a new rebased version.

[Caio Marcelo de Oliveira Filho]

Created bug 92137 for the non-mechanical bits of this change. I hope that bug is more "reviewable" :-)

[Caio Marcelo de Oliveira Filho]

Created attachment 155084 [details]
Patch for the EWS, not ready for review

[WebKit Review Bot]

Attachment 155084 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1
Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:319:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 363 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 155084 [details]
Patch for the EWS, not ready for review

Attachment 155084 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13372755

[Gyuyoung Kim]

Comment on attachment 155084 [details]
Patch for the EWS, not ready for review

Attachment 155084 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13381108

[Gyuyoung Kim]

Comment on attachment 155084 [details]
Patch for the EWS, not ready for review

Attachment 155084 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13380489

[Build Bot]

Comment on attachment 155084 [details]
Patch for the EWS, not ready for review

Attachment 155084 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13368931

[Caio Marcelo de Oliveira Filho]

Created attachment 155275 [details]
Patch for EWS

[WebKit Review Bot]

Attachment 155275 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1
Tools/DumpRenderTree/win/AccessibilityControllerWin.cpp:319:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 371 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 155275 [details]
Patch for EWS

Attachment 155275 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13392340

[WebKit Review Bot]

Comment on attachment 155275 [details]
Patch for EWS

Attachment 155275 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13396337

[Caio Marcelo de Oliveira Filho]

Created attachment 155317 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 155317 [details]
Patch

These are the mechanical changes only now, the most important thing to review is whether this API change for HashMap iterators is something we want or not.

[Caio Marcelo de Oliveira Filho]

Maciej, ping?

[Eric Seidel (no email)]

Comment on attachment 155317 [details]
Patch

LGTM.

[WebKit Review Bot]

Comment on attachment 155317 [details]
Patch

Rejecting attachment 155317 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ndle.cpp
patching file Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionNoCache_Bundle.cpp
patching file Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp
Hunk #1 FAILED at 452.
Hunk #2 FAILED at 467.
2 out of 2 hunks FAILED -- saving rejects to file Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Seidel']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13574146

[Caio Marcelo de Oliveira Filho]

Created attachment 160729 [details]
For EWS

[Caio Marcelo de Oliveira Filho]

Comment on attachment 160729 [details]
For EWS

Rebased patch to trunk, double checking builds with EWSs.

[Build Bot]

Comment on attachment 160729 [details]
For EWS

Attachment 160729 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13609407

[Gyuyoung Kim]

Comment on attachment 160729 [details]
For EWS

Attachment 160729 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13609416

[Gyuyoung Kim]

Comment on attachment 160729 [details]
For EWS

Attachment 160729 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13623096

[Caio Marcelo de Oliveira Filho]

Created attachment 160821 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160821 [details]
Patch

Rejecting attachment 160821 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
commit-queue/Source/WebKit/chromium/third_party/v8-i18n --revision 117 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 117.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13641065

[Caio Marcelo de Oliveira Filho]

Created attachment 160850 [details]
Patch for landing

[Caio Marcelo de Oliveira Filho]

Created attachment 160852 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 160852 [details]
Patch for landing

Rejecting attachment 160852 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Gardening: skipping tests due to WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) crashes.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/13636116

[WebKit Review Bot]

Comment on attachment 160852 [details]
Patch for landing

Clearing flags on attachment: 160852

Committed r126836: <http://trac.webkit.org/changeset/126836>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 95163

[Benjamin Poulain]

Looks like there are also many more hidden behind #ifdefs.

Please grep for ->first, ->second, ).first, ).second and check if they come from maps or pairs.

[Eric Seidel (no email)]

Sorry, we should have let the EWS chew on it first.  My bad.

[Gyuyoung Kim]

Comment on attachment 160852 [details]
Patch for landing

Attachment 160852 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13651040

[Caio Marcelo de Oliveira Filho]

This patch gets outdated too quickly, in this case a patch added code to JSC using first/second while CQ was processing it.

I'll land this manually tomorrow, and double check the potential cases behind development flags.

[Early Warning System Bot]

Comment on attachment 160852 [details]
Patch for landing

Attachment 160852 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13646189

[Early Warning System Bot]

Comment on attachment 160852 [details]
Patch for landing

Attachment 160852 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13656027

[Benjamin Poulain]

> This patch gets outdated too quickly, in this case a patch added code to JSC using first/second while CQ was processing it.
> 
> I'll land this manually tomorrow, and double check the potential cases behind development flags.

You can ping me before your land. I'll help you if the patch breaks the Mac bots again.

[Caio Marcelo de Oliveira Filho]

Committed r126914: <http://trac.webkit.org/changeset/126914>

[Simon Fraser (smfr)]

How can this massive patch have been landed with zero justification described in the changelog and commit message?

[WebKit Review Bot]

Re-opened since this is blocked by 95239

[Caio Marcelo de Oliveira Filho]

For the record, there was a discussion in the mailing list after the rollout.

http://lists.webkit.org/pipermail/webkit-dev/2012-August/022095.html

[Caio Marcelo de Oliveira Filho]

Created attachment 164222 [details]
For EWS consumption

[Caio Marcelo de Oliveira Filho]

Chromium is expected to fail in this last patch since now there's code in chromium repo (outside WebKit) that uses WTF::HashMap.

[WebKit Review Bot]

Comment on attachment 164222 [details]
For EWS consumption

Attachment 164222 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13841908

[Build Bot]

Comment on attachment 164222 [details]
For EWS consumption

Attachment 164222 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13845730

[Eric Seidel (no email)]

Comment on attachment 164222 [details]
For EWS consumption

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

> Source/WTF/wtf/HashTraits.h:209
> -        // TODO: Rename these to key and value. See https://bugs.webkit.org/show_bug.cgi?id=82784.
> -        KeyTypeArg first;
> -        ValueTypeArg second;
> +        KeyTypeArg key;
> +        ValueTypeArg value;

No luck with the fancy anonymous union stuff?

[Caio Marcelo de Oliveira Filho]

Comment on attachment 164222 [details]
For EWS consumption

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

>> Source/WTF/wtf/HashTraits.h:209
>> +        ValueTypeArg value;
> 
> No luck with the fancy anonymous union stuff?

It works but requires C++11 feature called "unrestricted unions". For Apple builds it is supported only in Clang >= 3.1. Since they still use a previous version, the union solution wouldn't help.

The current plan is: Benjamin will help me probably next week to land this.

[Peter Beverloo (cr-android ews)]

Comment on attachment 164222 [details]
For EWS consumption

Attachment 164222 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13841918

[Adrienne Walker]

(In reply to comment #60)
> Chromium is expected to fail in this last patch since now there's code in chromium repo (outside WebKit) that uses WTF::HashMap.

If you add "#define WTF_NEW_HASHMAP_ITERATORS_INTERFACE 1" to your patch, Chromium should compile after http://codereview.chromium.org/10914327/ lands.

Once this lands, and that revision rolls into Chromium, I'll take care of removing the #define.

[Caio Marcelo de Oliveira Filho]

Created attachment 164965 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164965 [details]
Patch

Attachment 164965 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13955297

[Peter Beverloo (cr-android ews)]

Comment on attachment 164965 [details]
Patch

Attachment 164965 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13975025

[James Robinson]

http://codereview.chromium.org/10914327/ has landed but hasn't rolled into WebKit yet, so this won't compile yet.  See Source/WebKit/chromium/DEPS

[Benjamin Poulain]

(In reply to comment #70)
> http://codereview.chromium.org/10914327/ has landed but hasn't rolled into WebKit yet, so this won't compile yet.  See Source/WebKit/chromium/DEPS

I am considering fixing this on Apple side today.
Can the patch be landed as is without breaking the Google side of the Universe?

[Adrienne Walker]

(In reply to comment #71)
> (In reply to comment #70)
> > http://codereview.chromium.org/10914327/ has landed but hasn't rolled into WebKit yet, so this won't compile yet.  See Source/WebKit/chromium/DEPS
> 
> I am considering fixing this on Apple side today.
> Can the patch be landed as is without breaking the Google side of the Universe?

The DEPS revision for Chromium has rolled past this change, so this patch should theoretically build cleanly on Chromium.

Please run it by the cr-linux EWS bot first.

[Benjamin Poulain]

> The DEPS revision for Chromium has rolled past this change, so this patch should theoretically build cleanly on Chromium.
> 
> Please run it by the cr-linux EWS bot first.

Next weekend I guess.

[Caio Marcelo de Oliveira Filho]

Created attachment 167381 [details]
Patch

[WebKit Review Bot]

Attachment 167381 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1
Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp:210:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/page/SpeechInput.cpp:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/chromium/NotificationPresenter.cpp:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/inspector/InspectorMemoryAgent.cpp:238:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/inspector/InspectorMemoryAgent.cpp:249:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 400 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 167381 [details]
Patch

Attachment 167381 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14172896

[Benjamin Poulain]

Created attachment 167488 [details]
Patch

[WebKit Review Bot]

Attachment 167488 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/API/JSCallbackObject..." exit_code: 1
Source/WebCore/svg/graphics/filters/SVGFilterBuilder.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGStructureCheckHoistingPhase.cpp:210:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/page/SpeechInput.cpp:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Tools/DumpRenderTree/chromium/NotificationPresenter.cpp:92:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/inspector/InspectorMemoryAgent.cpp:238:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/inspector/InspectorMemoryAgent.cpp:249:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 401 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 167488 [details]
Patch

Clearing flags on attachment: 167488

Committed r130612: <http://trac.webkit.org/changeset/130612>

[Benjamin Poulain]

All reviewed patches have been landed.  Closing bug.