Closed Bug 1212624 Opened 4 years ago Closed 4 years ago

Implement range-based iteration for LinkedList

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(4 files, 1 obsolete file)

The boilerplate, it boils me.

  for (MyType* e = snurf.snazzyList.getFirst(); e; e = e->getNext())
      e.yelp();
I think this is prettier:

  for (MyType* e : snurf.snazzyList)
      e.yelp();

and some people will think this is even prettier:

  for (auto e : snurf.snazzyList)
      e.yelp();
Attachment #8671035 - Flags: review?(jwalden+bmo)
Oops, pushed the wrong patch last time. That was just the test. This is the actual change. (Still getting used to mq-less bzexport.)
Attachment #8671039 - Flags: review?(jwalden+bmo)
Make use of the shiny new toys within Spidermonkey.
Attachment #8671040 - Flags: review?(jwalden+bmo)
And now, what got me started on doing this in the first place. Perhaps this should be in another bug.
Attachment #8671041 - Flags: review?(jwalden+bmo)
Not that it really matters for review, but it looks like I did one of those brilliant last-minute changes and removed a friend declaration that was, y'know, letting it compile.
Attachment #8671163 - Flags: review?(jwalden+bmo)
Attachment #8671040 - Attachment is obsolete: true
Attachment #8671040 - Flags: review?(jwalden+bmo)
Comment on attachment 8671035 [details] [diff] [review]
Tests for LinkedList

Review of attachment 8671035 [details] [diff] [review]:
-----------------------------------------------------------------

Coments apply to all the bits, not just the ones I noted, obviously.

::: mfbt/tests/TestLinkedList.cpp
@@ +10,5 @@
> +using mozilla::LinkedList;
> +using mozilla::LinkedListElement;
> +
> +struct SomeClass : public LinkedListElement<SomeClass> {
> +  int mValue;

unsigned, please.

@@ +26,5 @@
> +{
> +  LinkedList<SomeClass> list;
> +
> +  SomeClass one(1), two(2), three(3);
> +  

Remove all the trailing WS.

@@ +34,5 @@
> +  MOZ_RELEASE_ASSERT(!list.popFirst());
> +  MOZ_RELEASE_ASSERT(!list.popLast());
> +
> +  {
> +    int count = 0;

Use unsigned for these counts, please.

@@ +36,5 @@
> +
> +  {
> +    int count = 0;
> +    for (SomeClass* x : list) {
> +      x = x; // Suppress warning

MOZ_RELEASE_ASSERT(x, "can't have null elements in a list"); instead of this -- seems a slightly more sensible test.

@@ +87,5 @@
> +    }
> +  }
> +
> +  MOZ_RELEASE_ASSERT(list.getFirst()->mValue == 3);
> +  MOZ_RELEASE_ASSERT(list.getLast()->mValue == 4);

Add a couple removes (middle, then back, then front) and verify mValues and loop counts?  To cover the bases, mostly.

@@ +97,5 @@
> +  LinkedList<PrivateClass> list;
> +  PrivateClass one, two;
> +  list.insertBack(&one);
> +  list.insertBack(&two);
> +  for (PrivateClass* p : list) {

Blank line before the loop, and do a count == 2 check.
Attachment #8671035 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8671039 [details] [diff] [review]
Implement range-based iteration for LinkedList

Review of attachment 8671039 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/LinkedList.h
@@ +405,5 @@
>  
>    /*
> +   * Allow range-based iterator:
> +   *
> +   *     for (auto& elt : myList) { ... }

Is there a reason to encourage &-binding here?  I assume you get the usual bind-ref-to-temp lifetime extension behavior, so this is safe.  But it seems like it's generally safer to encourage creation/use of copies, and for people to do ref-binding as the exception.
Attachment #8671039 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8671041 [details] [diff] [review]
Make WeakMapBase be a LinkedListElement

Review of attachment 8671041 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsweakmap.cpp
@@ +123,1 @@
>          MOZ_ASSERT(m->isInList() && m->marked);

Brace this and split it into separate assertions, for debuggability?

::: js/src/jsweakmap.h
@@ +7,5 @@
>  #ifndef jsweakmap_h
>  #define jsweakmap_h
>  
>  #include "mozilla/Move.h"
> +#include "mozilla/LinkedList.h"

check-style no likey
Attachment #8671041 - Flags: review?(jwalden+bmo) → review+
Attachment #8671163 - Flags: review?(jwalden+bmo) → review+
Hmm.  So I think that link is more in the mood of assuming a person's going to question what to use in *every* case.  Which is good advice.  Maybe just remove the |auto| suggestion at all in that comment, so people aren't led into one particular thing and have to question?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> Comment on attachment 8671039 [details] [diff] [review]
> Implement range-based iteration for LinkedList
> 
> Review of attachment 8671039 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/LinkedList.h
> @@ +405,5 @@
> >  
> >    /*
> > +   * Allow range-based iterator:
> > +   *
> > +   *     for (auto& elt : myList) { ... }
> 
> Is there a reason to encourage &-binding here?  I assume you get the usual
> bind-ref-to-temp lifetime extension behavior, so this is safe.  But it seems
> like it's generally safer to encourage creation/use of copies, and for
> people to do ref-binding as the exception.

I was just wrong here. First, I personally think it's a bad idea to use auto at all here, because the documentation of the type is very important: if you were using range-based iteration over an array, you would be getting a reference to each element. Here, you are getting a *pointer* to each element. In all of the places I updated, it was much better to specify the exact type to make it clear what was going on.

And then there's the minor point that auto& doesn't even compile. Which is a good thing, since it would allow mutating the list structure by assigning to it. But instead, the compiler will barf about trying to cast a T* rvalue to a T*&.

Changed to

  /*
   * Allow range-based iteration:
   *
   *     for (MyElementType* elt : myList) { ... }
   */
IIRC you can just use use auto&& to avoid getting an lvalue (and avoid copies in some cases), though that's not really relevant for pointers.
Duplicate of this bug: 1215211
You need to log in before you can comment on or make changes to this bug.