Apply @harrishancock review comments.

c++/src/kj/hash.h

@@ -40,7 +40,7 @@ struct HashCoder {
   // a function like `hashCode(T)` and then rely on argument-dependent lookup.  However, this has
   // the problem that it pollutes other people's namespaces and even the global namespace.  For
   // example, some other project may already have functions called `hashCode` which do something
-  // different.  Declaring `operator*` with `Stringifier` as the left operand cannot conflict with
+  // different.  Declaring `operator*` with `HashCoder` as the left operand cannot conflict with
   // anything.
 
   uint operator*(ArrayPtr<const byte> s) const;
@@ -114,15 +114,16 @@ static KJ_CONSTEXPR(const) HashCoder HASHCODER = HashCoder();
 }  // namespace _ (private)
 
 #define KJ_HASHCODE(...) operator*(::kj::_::HashCoder, __VA_ARGS__)
-// Defines a stringifier for a custom type.  Example:
+// Defines a hash function for a custom type.  Example:
 //
 //    class Foo {...};
-//    inline StringPtr KJ_STRINGIFY(const Foo& foo) { return foo.name(); }
+//    inline uint KJ_HASHCODE(const Foo& foo) { return kj::hashCode(foo.x, foo.y); }
 //
-// This allows Foo to be passed to str().
+// This allows Foo to be passed to hashCode().
 //
 // The function should be declared either in the same namespace as the target type or in the global
-// namespace.  It can return any type which is an iterable container of chars.
+// namespace. It can return any type which itself is hashable -- that value will be hashed in turn
+// until a `uint` comes out.
 
 inline uint hashCode(uint value) { return value; }
 template <typename T>

c++/src/kj/map-test.c++

@@ -24,6 +24,7 @@
 
 namespace kj {
 namespace _ {
+namespace {
 
 KJ_TEST("HashMap") {
   HashMap<String, int> map;
@@ -147,5 +148,6 @@ KJ_TEST("TreeMap range") {
   }
 }
 
-}  // namespace kj
+}  // namespace
 }  // namespace _
+}  // namespace kj

c++/src/kj/map.h

@@ -36,7 +36,7 @@ class HashMap {
   //
   // `Key` must be hashable (via a `.hashCode()` method or `KJ_HASHCODE()`; see `hash.h`) and must
   // implement `operator==()`. Additionally, when performing lookups, you can use key types other
-  // that `Key` as long as the other type is also hashable (producing the same hash codes) and
+  // than `Key` as long as the other type is also hashable (producing the same hash codes) and
   // there is an `operator==` implementation with `Key` on the left and that other type on the
   // right. For example, if the key type is `String`, you can pass `StringPtr` to `find()`.
 

c++/src/kj/table-test.c++

@@ -27,6 +27,7 @@
 
 namespace kj {
 namespace _ {
+namespace {
 
 #if defined(KJ_DEBUG) && !__OPTIMIZE__
 static constexpr uint MEDIUM_PRIME = 619;
@@ -1100,5 +1101,6 @@ KJ_TEST("insertion order index") {
   }
 }
 
-}  // namespace kj
+}  // namespace
 }  // namespace _
+}  // namespace kj

c++/src/kj/table.c++

@@ -149,8 +149,6 @@ size_t chooseHashTableSize(uint size) {
 
 kj::Array<HashBucket> rehash(kj::ArrayPtr<const HashBucket> oldBuckets, size_t targetSize) {
   // Rehash the whole table.
-  // The element at `invalidPos` will be ignored.
-  // The element at `replacePos` will be recorded as if it were located at `invalidPos`.
 
   KJ_REQUIRE(targetSize < (1 << 30), "hash table has reached maximum size");
 
@@ -321,7 +319,8 @@ void BTreeImpl::growTree(uint minCapacity) {
 }
 
 BTreeImpl::Iterator BTreeImpl::search(const SearchKey& searchKey) const {
-  // Find the "first" row number (in sorted order) for which predicate(rowNumber) returns true.
+  // Find the "first" row number (in sorted order) for which searchKey.isAfter(rowNumber) returns
+  // false.
 
   uint pos = 0;
 
@@ -330,10 +329,8 @@ BTreeImpl::Iterator BTreeImpl::search(const SearchKey& searchKey) const {
     pos = parent.children[searchKey.search(parent)];
   }
 
-  {
-    auto& leaf = tree[pos].leaf;
-    return { tree, &leaf, searchKey.search(leaf) };
-  }
+  auto& leaf = tree[pos].leaf;
+  return { tree, &leaf, searchKey.search(leaf) };
 }
 
 template <typename T>
@@ -412,16 +409,14 @@ BTreeImpl::Iterator BTreeImpl::insert(const SearchKey& searchKey) {
     pos = node.children[indexInParent];
   }
 
-  {
-    Leaf& leaf = insertHelper(searchKey, tree[pos].leaf, parent, indexInParent, pos);
+  Leaf& leaf = insertHelper(searchKey, tree[pos].leaf, parent, indexInParent, pos);
 
-    // Fun fact: Unlike erase(), there's no need to climb back up the tree modifying keys, because
-    // either the newly-inserted node will not be the last in the leaf (and thus parent keys aren't
-    // modified), or the leaf is the last leaf in the tree (and thus there's no parent key to
-    // modify).
+  // Fun fact: Unlike erase(), there's no need to climb back up the tree modifying keys, because
+  // either the newly-inserted node will not be the last in the leaf (and thus parent keys aren't
+  // modified), or the leaf is the last leaf in the tree (and thus there's no parent key to
+  // modify).
 
-    return { tree, &leaf, searchKey.search(leaf) };
-  }
+  return { tree, &leaf, searchKey.search(leaf) };
 }
 
 template <typename Node>
@@ -510,24 +505,22 @@ void BTreeImpl::erase(uint row, const SearchKey& searchKey) {
     }
   }
 
-  {
-    Leaf& leaf = eraseHelper(tree[pos].leaf, parent, indexInParent, pos, fixup);
+  Leaf& leaf = eraseHelper(tree[pos].leaf, parent, indexInParent, pos, fixup);
 
-    uint r = searchKey.search(leaf);
-    if (leaf.rows[r] == row) {
-      leaf.erase(r);
+  uint r = searchKey.search(leaf);
+  if (leaf.rows[r] == row) {
+    leaf.erase(r);
 
-      if (fixup != nullptr) {
-        // There's a key in a parent node that needs fixup. This is only possible if the removed
-        // node is the last in its leaf.
-        KJ_DASSERT(leaf.rows[r] == nullptr);
-        KJ_DASSERT(r > 0);  // non-root nodes must be at least half full so this can't be item 0
-        KJ_DASSERT(*fixup == row);
-        *fixup = leaf.rows[r - 1];
-      }
-    } else {
-      logInconsistency();
+    if (fixup != nullptr) {
+      // There's a key in a parent node that needs fixup. This is only possible if the removed
+      // node is the last in its leaf.
+      KJ_DASSERT(leaf.rows[r] == nullptr);
+      KJ_DASSERT(r > 0);  // non-root nodes must be at least half full so this can't be item 0
+      KJ_DASSERT(*fixup == row);
+      *fixup = leaf.rows[r - 1];
     }
+  } else {
+    logInconsistency();
   }
 }
 
@@ -618,14 +611,12 @@ void BTreeImpl::renumber(uint oldRow, uint newRow, const SearchKey& searchKey) {
     KJ_DASSERT(pos != 0);
   }
 
-  {
-    auto& leaf = tree[pos].leaf;
-    uint r = searchKey.search(leaf);
-    if (leaf.rows[r] == oldRow) {
-      leaf.rows[r] = newRow;
-    } else {
-      logInconsistency();
-    }
+  auto& leaf = tree[pos].leaf;
+  uint r = searchKey.search(leaf);
+  if (leaf.rows[r] == oldRow) {
+    leaf.rows[r] = newRow;
+  } else {
+    logInconsistency();
   }
 }
 
@@ -711,7 +702,7 @@ void BTreeImpl::rotateLeft(
     Parent& left, Parent& right, Parent& parent, uint indexInParent, MaybeUint*& fixup) {
   // Steal one item from the right node and move it to the left node.
 
-  // Like mergeFrom(), this is only called on an exactly-half-empty node.
+  // Like merge(), this is only called on an exactly-half-empty node.
   KJ_DASSERT(left.isHalfFull());
   KJ_DASSERT(right.isMostlyFull());
 

c++/src/kj/table.h

@@ -160,6 +160,9 @@ public:
   void insertAll(Collection& collection);
   // Given an iterable collection of Rows, inserts all of them into this table. If the input is
   // an rvalue, the rows will be moved rather than copied.
+  //
+  // If an insertion throws (e.g. because it violates a uniqueness constraint of some index),
+  // subsequent insertions do not occur, but previous insertions remain inserted.
 
   template <typename UpdateFunc>
   Row& upsert(Row&& row, UpdateFunc&& update);
@@ -180,7 +183,7 @@ public:
   // Like find(), but if the row doesn't exist, call a function to create it. createFunc() must
   // return `Row` or something that implicitly converts to `Row`.
   //
-  // NOTE: C++ doesn't actually properly suppoprt inferring types of a parameter pack at the
+  // NOTE: C++ doesn't actually properly support inferring types of a parameter pack at the
   //   beginning of an argument list, but we define a hack to support it below. Don't worry about
   //   it.
 
@@ -308,7 +311,7 @@ class HashIndex;
 //     // methods to match this row.
 //
 //     bool matches(const Row&, SearchParams&&...) const;
-//     // Returns true if the the row on the left matches thes search params on the right.
+//     // Returns true if the row on the left matches thes search params on the right.
 //
 //     uint hashCode(SearchParams&&...) const;
 //     // Computes the hash code of the given search params. Matching rows (as determined by
@@ -318,7 +321,7 @@ class HashIndex;
 //   };
 //
 // If your `Callbacks` type has dynamic state, you may pass its constructor parameters as the
-// consturctor parameters to `HashIndex`.
+// constructor parameters to `HashIndex`.
 
 template <typename Callbacks>
 class TreeIndex;
@@ -401,7 +404,7 @@ private:
 
 template <typename Inner, typename Mapping>
 class MappedIterable: private Mapping {
-  // An iterator that wraps some other iterator and maps the values through a mapping function.
+  // An iterable that wraps some other iterable and maps the values through a mapping function.
   // The type `Mapping` must define a method `map()` which performs this mapping.
   //
   // TODO(cleanup): This seems generally useful. Should we put it somewhere resuable?
@@ -1074,18 +1077,18 @@ public:
   Iterator end() const;
 
   Iterator search(const SearchKey& searchKey) const;
-  // Find the "first" row number (in sorted order) for which predicate(rowNumber) returns false.
+  // Find the "first" row (in sorted order) for which searchKey.isAfter(rowNumber) returns true.
 
   Iterator insert(const SearchKey& searchKey);
   // Like search() but ensures that there is room in the leaf node to insert a new row.
 
   void erase(uint row, const SearchKey& searchKey);
-  // Erase the given row number from the tree. predicate() returns false for the given row and all
-  // rows after it.
+  // Erase the given row number from the tree. searchKey.isAfter() returns true for the given row
+  // and all rows after it.
 
   void renumber(uint oldRow, uint newRow, const SearchKey& searchKey);
-  // Renumber the given row from oldRow to newRow. predicate() returns false for oldRow and all
-  // rows after it. (It will not be called on newRow.)
+  // Renumber the given row from oldRow to newRow. searchKey.isAfter() returns true for oldRow and
+  // all rows after it. (It will not be called on newRow.)
 
   void verify(size_t size, FunctionParam<bool(uint, uint)>);
 
@@ -1436,7 +1439,7 @@ public:
   TreeIndex(Params&&... params): cb(kj::fwd<Params>(params)...) {}
 
   template <typename Row>
-  void verify(kj::ArrayPtr<Row> table) { // KJ_DBG
+  void verify(kj::ArrayPtr<Row> table) {
     impl.verify(table.size(), [&](uint i, uint j) {
       return cb.isBefore(table[i], table[j]);
     });