[webkit-dev] Proposal: WTF HashMap iterators to use key/value instead first/second

[Joe Mason]

I'm in favour of this change, as WTF::HashMap is not std::map and having the interface be almost like std::map in some ways, but totally unlike it in other ways, is more confusing than a clean break would be.
________________________________________
From: webkit-dev-bounces at lists.webkit.org [webkit-dev-bounces at lists.webkit.org] on behalf of Caio Marcelo de Oliveira Filho [caio.oliveira at openbossa.org]
Sent: Tuesday, August 28, 2012 6:24 PM
To: webkit-dev at lists.webkit.org
Subject: [webkit-dev] Proposal: WTF HashMap iterators to use key/value  instead first/second

Hello,

I would like to propose we change HashMap iterators to use 'key' and
'value' instead of 'first' and 'second'. The work is being tracked in
the bug https://bugs.webkit.org/show_bug.cgi?id=82784.

The idea came from https://bugs.webkit.org/show_bug.cgi?id=71063#c2. I
argue that this change will make code more readable, specially in
situations where either the key or the value is a pair. One example of
this situation is in
Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

             GetterSetterMap::AddResult result =
map.add(node->name().impl(), pair);
             if (!result.isNewEntry)
-                result.iterator->second.second = node;
+                result.iterator->value.second = node;

another one is in
Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp

     for (CCLayerTilingData::TileMap::const_iterator iter =
m_tiler->tiles().begin(); iter != m_tiler->tiles().end(); ++iter) {
-        int i = iter->first.first;
-        int j = iter->first.second;
-        UpdatableTile* tile = static_cast<UpdatableTile*>(iter->second.get());
+        int i = iter->key.first;
+        int j = iter->key.second;
+        UpdatableTile* tile = static_cast<UpdatableTile*>(iter->value.get());

I also think that in "non-pair key and non-pair value" situations the
readability is improved.

One downside of this change is that STL uses first and second in its
maps so that would confuse people. After inspecting the whole
repository for 'first' and 'second', the usage of std::map in WebKit
is an exception not a rule, IMHO this makes the confusion less
troublesome.

Are there other downsides am I missing? What do you guys think? Unless
we figure this is a bad change for the project, I intend to land it.

I've attempted to land this previously, I found out it caused a major
breakage in some internal builds. I apologize for that inconvenience
and the time lost.

I'm looking into ways to make the transition smoother, but until now
couldn't find a solution that doesn't require C++11. Either way, if we
land this change, I'll sync with developers from the affected ports
that I know could be internally affected to avoid more breakage.


Best regards,

--
Caio Marcelo de Oliveira Filho
openBossa @ INdT - Instituto Nokia de Tecnologia
_______________________________________________
webkit-dev mailing list
webkit-dev at lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-dev

---------------------------------------------------------------------
This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.