Review fixes.

search/retrieval.cpp

@@ -74,7 +74,7 @@ unique_ptr<coding::CompressedBitVector> RetrieveAddressFeaturesImpl(
   auto collector = [&](TValue const & value)
   {
     if (cancellable.IsCancelled())
-      throw CancelException();
+      MYTHROW(CancelException, ("Search cancelled"));
     features.push_back(value.m_featureId);
   };
 
@@ -97,7 +97,7 @@ unique_ptr<coding::CompressedBitVector> RetrieveGeometryFeatures(
   auto collector = [&](uint64_t featureId)
   {
     if (cancellable.IsCancelled())
-      throw CancelException();
+      MYTHROW(CancelException, ("Search cancelled"));
     features.push_back(featureId);
   };
 

search/retrieval.hpp

@@ -7,9 +7,9 @@
 #include "geometry/rect2d.hpp"
 
 #include "base/cancellable.hpp"
+#include "base/exception.hpp"
 #include "base/macros.hpp"
 
-#include "std/exception.hpp"
 #include "std/function.hpp"
 #include "std/unique_ptr.hpp"
 #include "std/vector.hpp"
@@ -29,9 +29,7 @@ namespace search
 //
 // TODO (@gorshenin): after merge to master, move this class to
 // base/cancellable.hpp.
-struct CancelException : public exception
-{
-};
+DECLARE_EXCEPTION(CancelException, RootException);
 
 class Retrieval : public my::Cancellable
 {

search/search_integration_tests/search_query_v2_test.cpp

@@ -52,10 +52,7 @@ UNIT_TEST(SearchQueryV2_Smoke)
   m2::RectD viewport(m2::PointD(-1.0, -1.0), m2::PointD(1.0, 1.0));
 
   Cleanup(map);
-  MY_SCOPE_GUARD(cleanup, [&]()
-                 {
-    Cleanup(map);
-  });
+  MY_SCOPE_GUARD(cleanup, [&]() { Cleanup(map); });
 
   vector<storage::CountryDef> countries;
   countries.emplace_back(map.GetCountryName(), viewport);
@@ -109,4 +106,10 @@ UNIT_TEST(SearchQueryV2_Smoke)
         make_shared<ExactMatch>(regResult.first, quantumTeleport1)};
     TEST(MatchResults(engine, rules, request.Results()), ());
   }
+
+  {
+    TestSearchRequest request(engine, "     ", "en", search::SearchParams::ALL, viewport);
+    request.Wait();
+    TEST_EQUAL(0, request.Results().size(), ());
+  }
 }

search/v2/features_layer_matcher.hpp

@@ -37,6 +37,8 @@ public:
       return;
     }
 
+    // TODO (y@): match POI with streets separately.
+
     vector<m2::PointD> childCenters;
     for (uint32_t featureId : child.m_features)
     {

search/v2/geocoder.cpp

@@ -48,6 +48,9 @@ void Geocoder::SetSearchQueryParams(SearchQueryParams const & params)
 
 void Geocoder::Go(vector<FeatureID> & results)
 {
+  if (m_numTokens == 0)
+    return;
+
   m_results = &results;
 
   try
@@ -69,7 +72,7 @@ void Geocoder::Go(vector<FeatureID> & results)
       m_mwmId = handle.GetId();
 
       MY_SCOPE_GUARD(cleanup, [&]()
-                     {
+      {
         m_finder.reset();
         m_loader.reset();
         m_cache.clear();
@@ -87,15 +90,18 @@ void Geocoder::Go(vector<FeatureID> & results)
   }
 }
 
-void Geocoder::PrepareParams(size_t from, size_t to)
+void Geocoder::PrepareParams(size_t curToken, size_t endToken)
 {
-  ASSERT_LESS(from, to, ());
-  ASSERT_LESS_OR_EQUAL(to, m_numTokens, ());
+  ASSERT_LESS(curToken, endToken, ());
+  ASSERT_LESS_OR_EQUAL(endToken, m_numTokens, ());
 
   m_retrievalParams.m_tokens.clear();
   m_retrievalParams.m_prefixTokens.clear();
 
-  for (size_t i = from; i < to; ++i)
+  // TODO (@y): possibly it's not cheap to copy vectors of strings.
+  // Profile it, and in case of serious performance loss, refactor
+  // SearchQueryParams to support subsets of tokens.
+  for (size_t i = curToken; i < endToken; ++i)
   {
     if (i < m_params.m_tokens.size())
       m_retrievalParams.m_tokens.push_back(m_params.m_tokens[i]);
@@ -108,8 +114,9 @@ void Geocoder::DoGeocoding(size_t curToken)
 {
   if (curToken == m_numTokens)
   {
-    // All tokens were consumed, intersect layers, emit features.
-    IntersectLayers();
+    // All tokens were consumed, find paths through layers, emit
+    // features.
+    FindPaths();
     return;
   }
 
@@ -127,6 +134,9 @@ void Geocoder::DoGeocoding(size_t curToken)
       layer.m_endToken = curToken + n;
     }
 
+    // TODO (@y, @m): as |n| increases, good optimization is to update
+    // |features| incrementally, from [curToken, curToken + n) to
+    // [curToken, curToken + n + 1).
     auto features = RetrieveAddressFeatures(curToken, curToken + n);
     if (!features || features->PopCount() == 0)
       continue;
@@ -208,7 +218,7 @@ bool Geocoder::LooksLikeHouseNumber(size_t curToken, size_t endToken) const
   return false;
 }
 
-void Geocoder::IntersectLayers()
+void Geocoder::FindPaths()
 {
   ASSERT(!m_layers.empty(), ());
 
@@ -225,7 +235,7 @@ void Geocoder::IntersectLayers()
   sort(sortedLayers.begin(), sortedLayers.end(), compareByType);
 
   m_finder->ForEachReachableVertex(sortedLayers, [this](uint32_t featureId)
-                                   {
+  {
     m_results->emplace_back(m_mwmId, featureId);
   });
 }

search/v2/geocoder.hpp

@@ -37,6 +37,21 @@ namespace v2
 class FeaturesLayerPathFinder;
 class SearchModel;
 
+// This class is used to retrieve all features corresponding to a
+// search query.  Search query is represented as a sequence of tokens
+// (including synonyms for these tokens), and Geocoder tries to build
+// all possible partitions (or layers) of the search query, where each
+// layer is a set of features corresponding to some search class
+// (e.g. POI, BUILDING, STREET, etc., see search/v2/search_model.hpp).
+// Then, Geocoder builds a layered graph, with edges between features
+// on adjacent layers (e.g. between BUILDING ans STREET, STREET and
+// CITY, etc.). Usually an edge between two features means that a
+// feature from the lowest layer geometrically belongs to a feature
+// from the highest layer (BUILDING is located on STREET, STREET is
+// located inside CITY, CITY is located inside STATE, etc.). Final
+// part is to find all paths through this layered graph and report all
+// features from the lowest layer, that are reachable from the highest
+// layer.
 class Geocoder : public my::Cancellable
 {
 public:
@@ -44,30 +59,46 @@ public:
 
   ~Geocoder() override;
 
+  // Sets search query params.
   void SetSearchQueryParams(SearchQueryParams const & params);
 
+  // Starts geocoding, retrieved features will be appended to
+  // |results|.
   void Go(vector<FeatureID> & results);
 
 private:
-  void PrepareParams(size_t from, size_t to);
+  // Fills |m_retrievalParams| with [curToken, endToken) subsequence
+  // of search query tokens.
+  void PrepareParams(size_t curToken, size_t endToken);
 
+  // Tries to find all paths in a search tree, where each edge is
+  // marked with some substring of the query tokens. These paths are
+  // called "layer sequence" and current path is stored in |m_layers|.
   void DoGeocoding(size_t curToken);
 
+  // Returns CBV of features corresponding to [curToken, endToken)
+  // subsequence of search query tokens. This method caches results of
+  // previous requests.
   coding::CompressedBitVector * RetrieveAddressFeatures(size_t curToken, size_t endToken);
 
+  // Returns true if current path in the search tree (see comment for
+  // DoGeocoding()) looks sane. This method is used as a fast
+  // pre-check to cut off unnecessary work.
   bool IsLayerSequenceSane() const;
 
+  // Returns true if [curToken, endToken) subsequence looks like a house number.
   bool LooksLikeHouseNumber(size_t curToken, size_t endToken) const;
 
-  void IntersectLayers();
+  // Finds all paths through layers and emits reachable features from
+  // the lowest layer.
+  void FindPaths();
 
   Index & m_index;
 
   // Initial search query params.
   SearchQueryParams m_params;
 
-  // Total number of tokens (including last prefix token, if
-  // non-empty).
+  // Total number of search query tokens.
   size_t m_numTokens;
 
   // This field is used to map features to a limited number of search
@@ -75,7 +106,7 @@ private:
   SearchModel const & m_model;
 
   // Following fields are set up by Search() method and can be
-  // modified and used only from Search() or it's callees.
+  // modified and used only from Search() or its callees.
 
   // Value of a current mwm.
   MwmValue * m_value;
@@ -83,7 +114,7 @@ private:
   // Id of a current mwm.
   MwmSet::MwmId m_mwmId;
 
-  // Cache of bit-vectors.
+  // Cache of posting list of features.
   unordered_map<uint64_t, unique_ptr<coding::CompressedBitVector>> m_cache;
 
   // Features loader.
@@ -98,6 +129,7 @@ private:
   // Stack of layers filled during geocoding.
   vector<FeaturesLayer> m_layers;
 
+  // Non-owning pointer to a vector of results.
   vector<FeatureID> * m_results;
 };
 }  // namespace v2

search/v2/search_model.hpp

@@ -14,7 +14,7 @@ namespace search
 {
 namespace v2
 {
-// This class is used to map our types to a restricted set of
+// This class is used to map feature types to a restricted set of
 // different search classes (do not confuse these classes with search
 // categories - they are completely different things).
 class SearchModel