Readability review

net/base/priority_queue.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_BASE_PRIORITY_QUEUE_H_
 #define NET_BASE_PRIORITY_QUEUE_H_
 #pragma once
 
 #include <list>
 #include <vector>
@@ skipping 16 matching lines @@
 // If the highest priority is 0, FirstMin() returns the first in order.
 //
 // In debug-mode, the internal queues store (id, value) pairs where id is used
 // to validate Pointers.
 //
 template<typename T>
 class PriorityQueue : public base::NonThreadSafe {
  private:
   // This section is up-front for Pointer only.
 #if !defined(NDEBUG)
   typedef std::list<std::pair<size_t, T> > List;

[stevez, 2012/04/11 16:13:58]

why size_t for the id?  it's not a size.

[szym, 2012/05/17 19:52:01]

Done.

 #else
   typedef std::list<T> List;
 #endif
 
  public:
   typedef uint32 Priority;
 
   // A pointer to a value stored in the queue. The pointer becomes invalid
   // when the queue is destroyed or cleared, or the value is erased.
   class Pointer {
    public:
     // Constructs a null pointer.
     Pointer() : priority_(kNullPriority) {

[stevez, 2012/04/11 16:13:58]

If you intend to make this copyable, define the copy constructor and assignment
operator; don't make maintainers wonder whether you just forgot to add
DISALLOW_COPY_AND_ASSIGN or not.

[szym, 2012/05/17 19:52:01]

The default copy constructor and assignment are fine. Would an explicit comment
suffice?

[stevez, 2012/05/22 14:22:40]

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Copy_Constructors#Cop...

"If a copy method is insufficient in your situation (e.g. for performance
reasons, or because your class needs to be stored by value in an STL container),
provide both a copy constructor and assignment operator."

http://www.corp.google.com/eng/doc/devguide/cpp/primer.html#disallow_copy_and...

"Any user-defined class that does not have value semantics (i.e., one that
cannot be safely assigned or copy-constructed) must either redefine operator=
and a copy constructor or hide those two methods so that the compiler does not
implicitly generate incorrect implementations of those behaviours."

[szym, 2012/05/23 01:26:49]

Understood. Done.

 #if !defined(NDEBUG)
       id_ = static_cast<size_t>(-1);
 #endif
     }
 
     bool is_null() const { return priority_ == kNullPriority; }
 
     Priority priority() const { return priority_; }
 
 #if !defined(NDEBUG)
@@ skipping 31 matching lines @@
     Priority priority_;
     ListIterator iterator_;
 
 #if !defined(NDEBUG)
     // Used by the queue to check if a Pointer is valid.
     size_t id_;
 #endif
   };
 
   // Creates a new queue for |num_priorities|.
   explicit PriorityQueue(Priority num_priorities)

[stevez, 2012/04/11 16:13:58]

Similarly, DISALLOW_COPY_OR_ASSIGN, or define copy constructor and assignment
operator.
(if you do make copyable, you should note what happens to Pointer's).

[szym, 2012/05/17 19:52:01]

Done.

       : lists_(num_priorities), size_(0) {
 #if !defined(NDEBUG)
     next_id_ = 0;
 #endif
   }
 
   // Adds |value| with |priority| to the queue. Returns a pointer to the
   // created element.
   Pointer Insert(const T& value, Priority priority) {
     DCHECK(CalledOnValidThread());
@@ skipping 23 matching lines @@
     DCHECK_EQ(pointer.iterator_->first, pointer.id_);
 #endif
 
     --size_;
     lists_[pointer.priority_].erase(pointer.iterator_);
   }
 
   // Returns a pointer to the first value of minimum priority or a null-pointer
   // if empty.
   Pointer FirstMin() {
     DCHECK(CalledOnValidThread());

[stevez, 2012/04/11 16:13:58]

For all these methods - is it worth checking size_ == 0 first?

[szym, 2012/05/17 19:52:01]

In the original review it was considered unnecessary.
http://codereview.chromium.org/8965025/#msg16
"It would be clearer to just remove this line and the NOTREACHED statement below
- I'm not too worried about perf here. Ditto for the rest of this file."

     for (size_t i = 0; i < lists_.size(); ++i) {
       if (!lists_[i].empty())
         return Pointer(i, lists_[i].begin());
     }
     return Pointer();
   }
 
   // Returns a pointer to the last value of minimum priority or a null-pointer
   // if empty.
   Pointer LastMin() {
@@ skipping 56 matching lines @@
 #endif
 
   ListVector lists_;
   size_t size_;
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_PRIORITY_QUEUE_H_
 
+