Bug 73370 – Hash* iterators should allow comparison between const and non-const versions

• Component: Web Template Framework
• Status: RESOLVED FIXED

[David Levin]

There is no reason to disallow this, and it is a constant source of confusion.

For example, 
  HashMap a;
  for (HashMap::const_iterator it = a.begin(); it != a.end(); ++it)

Generates a compile error because it and a.end() differ in the const iterator type.

[David Levin]

Created attachment 117060 [details]
Patch

[David Levin]

Perhaps there is a more concise way to do this but I don't know it.

I did this after hearing about another co-worker hit this issue and I remember someone else hitting this and not understanding it.

[David Levin]

Adding a comment from irc:

"One way it's nice that we don't allow comparisons is that it's really not the best way to iterate over a loop since it'll call end() for every step" -- andersca

[David Levin]

(In reply to comment #3)
> Adding a comment from irc:
> 
> "One way it's nice that we don't allow comparisons is that it's really not the best way to iterate over a loop since it'll call end() for every step" -- andersca

This is a good thing.

After more thought, it seemed like it still would be nice to have these comparison be available. Why?
1. The current compile error is confusing.
2. Even with omitting this, people are still able to write the loop if they use a non const_iterator.
3. It seems valuable to allow comparing iterator to const_iterator rather than forcing people to go through workarounds.
4. I like the idea of not having "end()" in the loop. It applies equally when using iterator alone and that is not stopped right now so this feels more like a coder/reviewer education issue.

[Alexey Proskuryakov]

> 2. Even with omitting this, people are still able to write the loop if they use a non const_iterator.

This is what I always do, precisely because plain iterator doesn't work!

[Alexey Proskuryakov]

Err, *because const_iterator doesn't work*.

Can't we generally expect optimizer to move the end() call out?

[Darin Adler]

Comment on attachment 117060 [details]
Patch

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

> Source/JavaScriptCore/wtf/HashTable.h:197
> +        bool operator==(const iterator& other) const
> +        {
> +            return *this == static_cast<const_iterator>(other);
> +        }
> +        bool operator!=(const iterator& other) const
> +        {
> +            return *this != static_cast<const_iterator>(other);
> +        }

We don’t have to change the type like this. We can just write:

    return get() == other.get();

[David Levin]

Committed as http://trac.webkit.org/changeset/102483

(In reply to comment #7)
> (From update of attachment 117060 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117060&action=review
> > Source/JavaScriptCore/wtf/HashTable.h:197
> > +        bool operator==(const iterator& other) const
> > +        {
> > +            return *this == static_cast<const_iterator>(other);
> > +        }
> We don’t have to change the type like this. We can just write:
> 
>     return get() == other.get();

I considered doing this but I felt bad that it would avoid the checkValidity() call done by the "bool operator==(const const_iterator& other) const" version. fwiw, the static_cast should only invoke the operator which returns the contained const_iterator.

If you disagree with this, I will change it to use get()

[Darin Adler]

(In reply to comment #8)
> I considered doing this but I felt bad that it would avoid the checkValidity() call done by the "bool operator==(const const_iterator& other) const" version.

I see.

> fwiw, the static_cast should only invoke the operator which returns the contained const_iterator.

Sure, but the problem with static_cast is that you can’t easily tell this for sure just looking at the call site. Anyway, I am fine with this as is.