Cheer up spell-checking code

chrome/browser/spellchecker/set_difference_container.h

+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[please use gerrit instead, 2016/02/03 23:59:11]

no (c)

[groby-ooo-7-16, 2016/02/04 03:43:15]

Do we have any reason to suspect a temporary set is so expensive that it
justifies adding 90+ lines of code for a new container type?

[Kevin Bailey, 2016/02/04 16:34:11]

Done, and fixed in the file(s) that I copied it from. :)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// A virtual container for iterating through the difference of two sets,
+// without having to construct a set.

[please use gerrit instead, 2016/02/03 23:59:11]

This is more of a class comment, so let's move it above the class. A file
comment would be useful to describe multiple, non-nested classes within a file.
Also would be nice to provide a code snippet of sample usage in the class
comment.

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+
+#ifndef CHROME_BROWSER_SPELLCHECKER_SET_DIFFERENCE_CONTAINER_H_
+#define CHROME_BROWSER_SPELLCHECKER_SET_DIFFERENCE_CONTAINER_H_
+
+namespace spellcheck {
+
+template <typename Container1, typename Container2>
+class set_difference_container {

[please use gerrit instead, 2016/02/03 23:59:10]

Class names are usually CamelCase. Is there a precedent that you're following?

[Kevin Bailey, 2016/02/04 16:34:11]

Only standard C++ habit. Done.

+ public:
+  set_difference_container(
+      const Container1& container1,
+      const Container2& container2)
+      : in_beg(container1.begin()), in_end(container1.end()),
+        out_beg(container2.begin()), out_end(container2.end()) {}

[please use gerrit instead, 2016/02/03 23:59:10]

Delegate the construction to the second constructor for simplicity.

[Kevin Bailey, 2016/02/04 16:34:11]

Good idea. Done.

+
+  set_difference_container(

[please use gerrit instead, 2016/02/03 23:59:10]

If you have a constructor, then you should have a destructor too, according to
our style conventions.

[Kevin Bailey, 2016/02/04 16:34:11]

I went through 3 style guides and the only thing mentioned about destructors
was, of course, that you must have a virtual destructor in a virtual class, and
that you don't want it in-line for a number of reasons, but nothing about
mandating declaring it. Can you point me to this? 

+      typename Container1::const_iterator x_beg,
+      typename Container1::const_iterator x_end,
+      typename Container2::const_iterator y_beg,
+      typename Container2::const_iterator y_end)
+      : in_beg(x_beg), in_end(x_end), out_beg(y_beg), out_end(y_end) {}

[please use gerrit instead, 2016/02/03 23:59:11]

Do something similar to
https://code.google.com/p/chromium/codesearch#chromium/src/base/stl_util.h&rc...
to check that the data is sorted.

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+
+  class set_difference_iterator {

[please use gerrit instead, 2016/02/03 23:59:11]

Internal classes usually come before constructors.

[Kevin Bailey, 2016/02/04 16:34:12]

Done.

+   public:
+    set_difference_iterator() {}
+
+    set_difference_iterator(

[please use gerrit instead, 2016/02/03 23:59:11]

Need a destructor too.

[Kevin Bailey, 2016/02/04 16:34:11]

Acknowledged.

+        typename Container1::const_iterator x_beg,
+        typename Container1::const_iterator x_end,
+        typename Container2::const_iterator y_beg,
+        typename Container2::const_iterator y_end)
+      : in_beg(x_beg), in_end(x_end), out_beg(y_beg), out_end(y_end) {
+      // go to first valid value
+      MaybeIterate();
+    }
+
+    void MaybeIterate() {
+      while (in_beg != in_end && out_beg != out_end && *out_beg <= *in_beg) {
+        // skip over stuff that is equal
+        if (*in_beg == *out_beg)
+          ++in_beg;
+        // go to next in |out| that is >= |in|
+        ++out_beg;
+      }
+    }
+
+    set_difference_iterator operator++() {

[please use gerrit instead, 2016/02/03 23:59:10]

Return a reference to avoid a copy.

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+      // assert in_beg != in_end

[please use gerrit instead, 2016/02/03 23:59:10]

#include "base/logging.h"

DCHECK_NE(in_beg, in_end);

[Kevin Bailey, 2016/02/04 16:34:11]

Sorry, didn't work, I suspect, because it was attempting to stream an iterator.
If you know a way to fix that, happy to include it.

+      ++in_beg;
+      MaybeIterate();
+      return *this;
+    }
+
+    const typename Container1::value_type& operator*() {
+      // assert in_beg != in_end
+      return *in_beg;
+    }
+
+    bool operator!=(const set_difference_iterator& rhs) {
+      // This is completely cutting corners, but should work.
+      // |out| isn't really part of the state, and we just trust
+      // |end| to be correct.
+      return in_beg != rhs.in_beg;
+    }
+   private:

[please use gerrit instead, 2016/02/03 23:59:10]

newline above private.

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+    typename Container1::const_iterator in_beg, in_end;
+    typename Container2::const_iterator out_beg, out_end;
+  };

[please use gerrit instead, 2016/02/03 23:59:10]

newline below };

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+  typedef set_difference_iterator iterator;
+
+  iterator begin() const {
+    return iterator(in_beg, in_end, out_beg, out_end);
+  }
+
+  iterator end() const {
+    return iterator(in_end, in_end, out_end, out_end);
+  }
+
+ private:
+  typename Container1::const_iterator in_beg, in_end;

[please use gerrit instead, 2016/02/03 23:59:10]

Each member variable on its own line, even if the type is the same.

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+  typename Container2::const_iterator out_beg, out_end;

[please use gerrit instead, 2016/02/03 23:59:10]

Ditto.

[Kevin Bailey, 2016/02/04 16:34:11]

Done.

+};
+
+}  // namespace spellcheck
+
+#endif  // CHROME_BROWSER_SPELLCHECKER_SET_DIFFERENCE_CONTAINER_H_