From 7a37590888d6f62537b1239f53b398fe853277c1 Mon Sep 17 00:00:00 2001 From: Maxim Pimenov Date: Wed, 25 Oct 2017 16:46:03 +0300 Subject: [PATCH] Review fixes. --- indexer/search_string_utils.cpp | 5 +- indexer/search_string_utils.hpp | 4 +- .../Content/BookingCells/MWMPPReviewCell.mm | 2 +- search/common.hpp | 3 +- search/highlighting.cpp | 5 +- search/intermediate_result.cpp | 227 +++++++++--------- search/intermediate_result.hpp | 36 ++- search/pre_ranker.cpp | 8 +- search/processor.hpp | 5 +- search/ranker.cpp | 26 +- .../search_quality_tool.cpp | 3 +- search/search_tests/highlighting_tests.cpp | 20 +- search/suggest.cpp | 2 +- 13 files changed, 172 insertions(+), 174 deletions(-) diff --git a/indexer/search_string_utils.cpp b/indexer/search_string_utils.cpp index c1ecfaa816..231231f317 100644 --- a/indexer/search_string_utils.cpp +++ b/indexer/search_string_utils.cpp @@ -298,7 +298,7 @@ private: StreetsSynonymsHolder g_streets; } // namespace -void GetStringPrefix(string const & str, string & res) +string DropLastToken(string const & str) { search::Delimiters delims; using Iter = utf8::unchecked::iterator; @@ -316,8 +316,7 @@ void GetStringPrefix(string const & str, string & res) iter = prev; } - // Assign the input string without prefix to result. - res.assign(str.begin(), iter.base()); + return string(str.begin(), iter.base()); } UniString GetStreetNameAsKey(string const & name) diff --git a/indexer/search_string_utils.hpp b/indexer/search_string_utils.hpp index cad9a1e361..a8692d60f0 100644 --- a/indexer/search_string_utils.hpp +++ b/indexer/search_string_utils.hpp @@ -55,8 +55,8 @@ bool TokenizeStringAndCheckIfLastTokenIsPrefix(std::string const & s, Tokens & t return TokenizeStringAndCheckIfLastTokenIsPrefix(NormalizeAndSimplifyString(s), tokens, delims); } -// Chops off the last query token (the "prefix" one) from |str| and stores the result in |res|. -void GetStringPrefix(std::string const & str, std::string & res); +// Chops off the last query token (the "prefix" one) from |str|. +std::string DropLastToken(std::string const & str); strings::UniString GetStreetNameAsKey(std::string const & name); diff --git a/iphone/Maps/UI/PlacePage/PlacePageLayout/Content/BookingCells/MWMPPReviewCell.mm b/iphone/Maps/UI/PlacePage/PlacePageLayout/Content/BookingCells/MWMPPReviewCell.mm index 1bc9dba24c..02e342f256 100644 --- a/iphone/Maps/UI/PlacePage/PlacePageLayout/Content/BookingCells/MWMPPReviewCell.mm +++ b/iphone/Maps/UI/PlacePage/PlacePageLayout/Content/BookingCells/MWMPPReviewCell.mm @@ -33,7 +33,7 @@ formatter.dateStyle = NSDateFormatterLongStyle; self.date.text = [formatter stringFromDate: - [NSDate dateWithTimeIntervalSince1970:duration_cast(review.m_date.time_since_epoch()).count()]]; + [NSDate dateWithTimeIntervalSince1970:std::chrono::duration_cast(review.m_date.time_since_epoch()).count()]]; self.positiveReview.text = @(review.m_pros.c_str()); self.negativeReview.text = @(review.m_cons.c_str()); diff --git a/search/common.hpp b/search/common.hpp index c735c4e0ad..37839e5c0d 100644 --- a/search/common.hpp +++ b/search/common.hpp @@ -13,7 +13,8 @@ namespace search // the prefix and non-prefix tokens. using QueryTokens = buffer_vector; -using Locales = base::SafeSmallSet; +using Locales = + base::SafeSmallSet(CategoriesHolder::kMaxSupportedLocaleIndex) + 1>; /// Upper bound for max count of tokens for indexing and scoring. int constexpr MAX_TOKENS = 32; diff --git a/search/highlighting.cpp b/search/highlighting.cpp index bfaee0e4ab..cd92942c8c 100644 --- a/search/highlighting.cpp +++ b/search/highlighting.cpp @@ -44,14 +44,13 @@ public: namespace search { -// static void HighlightResult(QueryTokens const & tokens, strings::UniString const & prefix, Result & res) { using Iter = QueryTokens::const_iterator; using CombinedIter = CombinedIterator; - CombinedIter beg(tokens.begin(), tokens.end(), prefix.empty() ? 0 : &prefix); - CombinedIter end(tokens.end() /* cur */, tokens.end() /* end */, 0); + CombinedIter beg(tokens.begin(), tokens.end(), prefix.empty() ? nullptr : &prefix); + CombinedIter end(tokens.end() /* cur */, tokens.end() /* end */, nullptr); auto assignHighlightRange = [&](pair const & range) { res.AddHighlightRange(range); }; diff --git a/search/intermediate_result.cpp b/search/intermediate_result.cpp index 71f55a89ac..dca4c26dad 100644 --- a/search/intermediate_result.cpp +++ b/search/intermediate_result.cpp @@ -19,16 +19,22 @@ #include "base/string_utils.hpp" #include "base/logging.hpp" +#include +#include + #include "3party/opening_hours/opening_hours.hpp" namespace search { namespace { +char const * const kEmptyRatingSymbol = "-"; +char const * const kPricingSymbol = "$"; + class SkipRegionInfo { - static size_t const m_count = 2; - uint32_t m_types[m_count]; + static size_t const kCount = 2; + uint32_t m_types[kCount]; public: SkipRegionInfo() @@ -37,10 +43,10 @@ public: {"place", "continent"}, {"place", "country"} }; - static_assert(m_count == ARRAY_SIZE(arr), ""); + static_assert(kCount == ARRAY_SIZE(arr), ""); Classificator const & c = classif(); - for (size_t i = 0; i < m_count; ++i) + for (size_t i = 0; i < kCount; ++i) m_types[i] = c.GetTypeByPath(vector(arr[i], arr[i] + 2)); } @@ -56,9 +62,107 @@ public: }; } // namespace -char const * const kEmptyRatingSymbol = "-"; -char const * const kPricingSymbol = "$"; +// PreRankerResult --------------------------------------------------------------------------------- +PreRankerResult::PreRankerResult(FeatureID const & id, PreRankingInfo const & info) + : m_id(id), m_info(info) +{ + ASSERT(m_id.IsValid(), ()); +} +// static +bool PreRankerResult::LessRank(PreRankerResult const & r1, PreRankerResult const & r2) +{ + if (r1.m_info.m_rank != r2.m_info.m_rank) + return r1.m_info.m_rank > r2.m_info.m_rank; + return r1.m_info.m_distanceToPivot < r2.m_info.m_distanceToPivot; +} + +// static +bool PreRankerResult::LessDistance(PreRankerResult const & r1, PreRankerResult const & r2) +{ + if (r1.m_info.m_distanceToPivot != r2.m_info.m_distanceToPivot) + return r1.m_info.m_distanceToPivot < r2.m_info.m_distanceToPivot; + return r1.m_info.m_rank > r2.m_info.m_rank; +} + +// RankerResult ------------------------------------------------------------------------------------ +RankerResult::RankerResult(FeatureType const & f, m2::PointD const & center, + m2::PointD const & pivot, string const & displayName, + string const & fileName) + : m_id(f.GetID()) + , m_types(f) + , m_str(displayName) + , m_resultType(ftypes::IsBuildingChecker::Instance()(m_types) ? TYPE_BUILDING : TYPE_FEATURE) + , m_geomType(f.GetFeatureType()) +{ + ASSERT(m_id.IsValid(), ()); + ASSERT(!m_types.Empty(), ()); + + m_types.SortBySpec(); + + m_region.SetParams(fileName, center); + m_distance = PointDistance(center, pivot); + + ProcessMetadata(f, m_metadata); +} + +RankerResult::RankerResult(double lat, double lon) + : m_str("(" + measurement_utils::FormatLatLon(lat, lon) + ")"), m_resultType(TYPE_LATLON) +{ + m_region.SetParams(string(), MercatorBounds::FromLatLon(lat, lon)); +} + +string RankerResult::GetRegionName(storage::CountryInfoGetter const & infoGetter, + uint32_t ftype) const +{ + static SkipRegionInfo const checker; + if (checker.IsSkip(ftype)) + return string(); + + storage::CountryInfo info; + m_region.GetRegion(infoGetter, info); + return info.m_name; +} + +bool RankerResult::IsEqualCommon(RankerResult const & r) const +{ + return m_geomType == r.m_geomType && GetBestType() == r.GetBestType() && m_str == r.m_str; +} + +bool RankerResult::IsStreet() const +{ + return m_geomType == feature::GEOM_LINE && ftypes::IsStreetChecker::Instance()(m_types); +} + +uint32_t RankerResult::GetBestType(set const * pPrefferedTypes) const +{ + if (pPrefferedTypes) + { + for (uint32_t type : m_types) + { + if (pPrefferedTypes->count(type) > 0) + return type; + } + } + + // Do type truncate (2-level is enough for search results) only for + // non-preffered types (types from categories leave original). + uint32_t type = m_types.GetBestType(); + ftype::TruncValue(type, 2); + return type; +} + +// RankerResult::RegionInfo ------------------------------------------------------------------------ +void RankerResult::RegionInfo::GetRegion(storage::CountryInfoGetter const & infoGetter, + storage::CountryInfo & info) const +{ + if (!m_file.empty()) + infoGetter.GetRegionInfo(m_file, info); + else + infoGetter.GetRegionInfo(m_point, info); +} + +// Functions --------------------------------------------------------------------------------------- void ProcessMetadata(FeatureType const & ft, Result::Metadata & meta) { if (meta.m_isInitialized) @@ -108,112 +212,15 @@ void ProcessMetadata(FeatureType const & ft, Result::Metadata & meta) meta.m_isInitialized = true; } -PreRankerResult::PreRankerResult(FeatureID const & fID, PreRankingInfo const & info) - : m_id(fID), m_info(info) -{ - ASSERT(m_id.IsValid(), ()); -} - -// static -bool PreRankerResult::LessRank(PreRankerResult const & r1, PreRankerResult const & r2) -{ - if (r1.m_info.m_rank != r2.m_info.m_rank) - return r1.m_info.m_rank > r2.m_info.m_rank; - return r1.m_info.m_distanceToPivot < r2.m_info.m_distanceToPivot; -} - -// static -bool PreRankerResult::LessDistance(PreRankerResult const & r1, PreRankerResult const & r2) -{ - if (r1.m_info.m_distanceToPivot != r2.m_info.m_distanceToPivot) - return r1.m_info.m_distanceToPivot < r2.m_info.m_distanceToPivot; - return r1.m_info.m_rank > r2.m_info.m_rank; -} - -RankerResult::RankerResult(FeatureType const & f, m2::PointD const & center, - m2::PointD const & pivot, string const & displayName, - string const & fileName) - : m_id(f.GetID()) - , m_types(f) - , m_str(displayName) - , m_resultType(ftypes::IsBuildingChecker::Instance()(m_types) ? TYPE_BUILDING : TYPE_FEATURE) - , m_geomType(f.GetFeatureType()) -{ - ASSERT(m_id.IsValid(), ()); - ASSERT(!m_types.Empty(), ()); - - m_types.SortBySpec(); - - m_region.SetParams(fileName, center); - m_distance = PointDistance(center, pivot); - - ProcessMetadata(f, m_metadata); -} - -RankerResult::RankerResult(double lat, double lon) - : m_str("(" + measurement_utils::FormatLatLon(lat, lon) + ")"), m_resultType(TYPE_LATLON) -{ - m_region.SetParams(string(), MercatorBounds::FromLatLon(lat, lon)); -} - -string RankerResult::GetRegionName(storage::CountryInfoGetter const & infoGetter, - uint32_t fType) const -{ - static SkipRegionInfo const checker; - if (checker.IsSkip(fType)) - return string(); - - storage::CountryInfo info; - m_region.GetRegion(infoGetter, info); - return info.m_name; -} - -bool RankerResult::IsEqualCommon(RankerResult const & r) const -{ - return m_geomType == r.m_geomType && GetBestType() == r.GetBestType() && m_str == r.m_str; -} - -bool RankerResult::IsStreet() const -{ - return m_geomType == feature::GEOM_LINE && ftypes::IsStreetChecker::Instance()(m_types); -} - -string RankerResult::DebugPrint() const +string DebugPrint(RankerResult const & r) { stringstream ss; - ss << "IntermediateResult [ " - << "Name: " << m_str - << "; Type: " << GetBestType() - << "; " << search::DebugPrint(m_info) - << "; Linear model rank: " << m_info.GetLinearModelRank() - << " ]"; + ss << "RankerResult [" + << "Name: " << r.GetName() + << "; Type: " << r.GetBestType() + << "; " << DebugPrint(r.GetRankingInfo()) + << "; Linear model rank: " << r.GetLinearModelRank() + << "]"; return ss.str(); } - -uint32_t RankerResult::GetBestType(set const * pPrefferedTypes) const -{ - if (pPrefferedTypes) - { - for (uint32_t type : m_types) - { - if (pPrefferedTypes->count(type) > 0) - return type; - } - } - - // Do type truncate (2-level is enough for search results) only for - // non-preffered types (types from categories leave original). - uint32_t type = m_types.GetBestType(); - ftype::TruncValue(type, 2); - return type; -} - -void RankerResult::RegionInfo::GetRegion(storage::CountryInfoGetter const & infoGetter, - storage::CountryInfo & info) const -{ - if (!m_file.empty()) - infoGetter.GetRegionInfo(m_file, info); - else - infoGetter.GetRegionInfo(m_point, info); -} } // namespace search diff --git a/search/intermediate_result.hpp b/search/intermediate_result.hpp index c16fa1e062..989c1e91d8 100644 --- a/search/intermediate_result.hpp +++ b/search/intermediate_result.hpp @@ -27,7 +27,7 @@ class ReverseGeocoder; class PreRankerResult { public: - PreRankerResult(FeatureID const & fID, PreRankingInfo const & info); + PreRankerResult(FeatureID const & id, PreRankingInfo const & info); static bool LessRank(PreRankerResult const & r1, PreRankerResult const & r2); static bool LessDistance(PreRankerResult const & r1, PreRankerResult const & r2); @@ -64,7 +64,9 @@ public: /// For RESULT_LATLON. RankerResult(double lat, double lon); - inline search::RankingInfo const & GetRankingInfo() const { return m_info; } + bool IsStreet() const; + + search::RankingInfo const & GetRankingInfo() const { return m_info; } template inline void SetRankingInfo(TInfo && info) @@ -72,23 +74,19 @@ public: m_info = forward(info); } - string DebugPrint() const; + FeatureID const & GetID() const { return m_id; } + string const & GetName() const { return m_str; } + feature::TypesHolder const & GetTypes() const { return m_types; } + Type const & GetResultType() const { return m_resultType; } + m2::PointD GetCenter() const { return m_region.m_point; } + double GetDistance() const { return m_distance; } + feature::EGeomType GetGeomType() const { return m_geomType; } + Result::Metadata GetMetadata() const { return m_metadata; } - bool IsStreet() const; + double GetDistanceToPivot() const { return m_info.m_distanceToPivot; } + double GetLinearModelRank() const { return m_info.GetLinearModelRank(); } - inline FeatureID const & GetID() const { return m_id; } - inline string const & GetName() const { return m_str; } - inline feature::TypesHolder const & GetTypes() const { return m_types; } - inline Type const & GetResultType() const { return m_resultType; } - inline m2::PointD GetCenter() const { return m_region.m_point; } - inline double GetDistance() const { return m_distance; } - inline feature::EGeomType GetGeomType() const { return m_geomType; } - inline Result::Metadata GetMetadata() const { return m_metadata; } - - inline double GetDistanceToPivot() const { return m_info.m_distanceToPivot; } - inline double GetLinearModelRank() const { return m_info.GetLinearModelRank(); } - - string GetRegionName(storage::CountryInfoGetter const & infoGetter, uint32_t fType) const; + string GetRegionName(storage::CountryInfoGetter const & infoGetter, uint32_t ftype) const; bool IsEqualCommon(RankerResult const & r) const; @@ -123,7 +121,7 @@ private: Result::Metadata m_metadata; }; -inline string DebugPrint(RankerResult const & t) { return t.DebugPrint(); } - void ProcessMetadata(FeatureType const & ft, Result::Metadata & meta); + +string DebugPrint(RankerResult const & r); } // namespace search diff --git a/search/pre_ranker.cpp b/search/pre_ranker.cpp index 4b470c4cca..2052e8aac2 100644 --- a/search/pre_ranker.cpp +++ b/search/pre_ranker.cpp @@ -13,7 +13,10 @@ #include "base/random.hpp" #include "base/stl_helpers.hpp" -#include "std/iterator.hpp" +#include +#include + +using namespace std; namespace search { @@ -175,8 +178,7 @@ void PreRanker::Filter(bool viewportSearch) } } - using TSet = set; - TSet filtered; + set filtered; filtered.insert(m_results.begin(), m_results.begin() + min(m_results.size(), BatchSize())); diff --git a/search/processor.hpp b/search/processor.hpp index c09df8b3f8..61f0952196 100644 --- a/search/processor.hpp +++ b/search/processor.hpp @@ -135,11 +135,8 @@ protected: void SetViewportByIndex(m2::RectD const & viewport, size_t idx, bool forceUpdate); void ClearCache(size_t ind); - /// @name Get ranking params. - //@{ - /// @return Rect for viewport-distance calculation. + // Returns a Rect for viewport-distance calculations. m2::RectD const & GetViewport(ViewportID vID = DEFAULT_V) const; - //@} void SetLanguage(int id, int8_t lang); int8_t GetLanguage(int id) const; diff --git a/search/ranker.cpp b/search/ranker.cpp index ae341a4b5c..3408fc4ec6 100644 --- a/search/ranker.cpp +++ b/search/ranker.cpp @@ -16,6 +16,8 @@ #include "std/iterator.hpp" #include "std/unique_ptr.hpp" +#include + namespace search { namespace @@ -83,7 +85,7 @@ void RemoveDuplicatingLinear(vector & results) if (t1 != t2) return t1 < t2; - // Should stay the best feature, after unique, so add this criteria: + // After unique, the better feature should be kept. return r1.GetDistance() < r2.GetDistance(); }; @@ -119,7 +121,6 @@ ftypes::Type GetLocalityIndex(feature::TypesHolder const & types) // TODO: Format street and house number according to local country's rules. string FormatStreetAndHouse(ReverseGeocoder::Address const & addr) { - ASSERT_GREATER_OR_EQUAL(addr.GetDistance(), 0, ()); return addr.GetStreetName() + ", " + addr.GetHouseNumber(); } @@ -276,14 +277,13 @@ class RankerResultMaker } public: - explicit RankerResultMaker(Ranker & ranker, Index const & index, - storage::CountryInfoGetter const & infoGetter, - Geocoder::Params const & params) + RankerResultMaker(Ranker & ranker, Index const & index, + storage::CountryInfoGetter const & infoGetter, Geocoder::Params const & params) : m_ranker(ranker), m_index(index), m_params(params), m_infoGetter(infoGetter) { } - unique_ptr operator()(PreRankerResult const & preRankerResult) + boost::optional operator()(PreRankerResult const & preRankerResult) { FeatureType ft; m2::PointD center; @@ -293,15 +293,14 @@ public: if (!LoadFeature(preRankerResult.GetId(), ft, center, name, country)) return {}; - auto p = make_unique(ft, center, m_ranker.m_params.m_position /* pivot */, name, - country); + RankerResult r(ft, center, m_ranker.m_params.m_position /* pivot */, name, country); search::RankingInfo info; InitRankingInfo(ft, center, preRankerResult, info); info.m_rank = NormalizeRank(info.m_rank, info.m_type, center, country); - p->SetRankingInfo(move(info)); + r.SetRankingInfo(move(info)); - return p; + return r; } }; @@ -352,8 +351,7 @@ void Ranker::SuggestStrings() if (m_params.m_prefix.empty() || !m_params.m_suggestsEnabled) return; - string prologue; - GetStringPrefix(m_params.m_query, prologue); + string prologue = DropLastToken(m_params.m_query); for (auto const & locale : m_params.m_categoryLocales) MatchForSuggestions(m_params.m_prefix, locale, prologue); @@ -438,7 +436,7 @@ void Ranker::MakeRankerResults(Geocoder::Params const & geocoderParams, } if (!ResultExists(*p, results, m_params.m_minDistanceOnMapBetweenResults)) - results.push_back(move(*p.release())); + results.push_back(move(*p)); }; } @@ -516,7 +514,7 @@ void Ranker::MatchForSuggestions(strings::UniString const & token, int8_t locale if (suggest.m_prefixLength <= token.size() && token != s // do not push suggestion if it already equals to token && suggest.m_locale == locale // push suggestions only for needed language - && strings::StartsWith(s.begin(), s.end(), token.begin(), token.end())) + && strings::StartsWith(s, token)) { string const utf8Str = strings::ToUtf8(s); Result r(utf8Str, prologue + utf8Str + " "); diff --git a/search/search_quality/search_quality_tool/search_quality_tool.cpp b/search/search_quality/search_quality_tool/search_quality_tool.cpp index 66ab53db41..ba7f16d22f 100644 --- a/search/search_quality/search_quality_tool/search_quality_tool.cpp +++ b/search/search_quality/search_quality_tool/search_quality_tool.cpp @@ -378,8 +378,7 @@ int main(int argc, char * argv[]) Engine::Params params; params.m_locale = FLAGS_locale; params.m_numThreads = FLAGS_num_threads; - TestSearchEngine engine(move(infoGetter), make_unique(), Engine::Params{}); - engine.SetLocale(FLAGS_locale); + TestSearchEngine engine(move(infoGetter), make_unique(), params); vector mwms; if (!FLAGS_mwm_list_path.empty()) diff --git a/search/search_tests/highlighting_tests.cpp b/search/search_tests/highlighting_tests.cpp index 24090f11f7..75f1ec4af1 100644 --- a/search/search_tests/highlighting_tests.cpp +++ b/search/search_tests/highlighting_tests.cpp @@ -5,14 +5,17 @@ #include "indexer/feature_covering.hpp" #include "base/logging.hpp" +#include "base/string_utils.hpp" #include "std/cstdarg.hpp" +#include "std/string.hpp" +#include "std/vector.hpp" namespace { -typedef pair TestResult; -typedef vector TokensVector; -typedef vector TestResultVector; +using TestResult = pair; +using TokensVector = vector; +using TestResultVector = vector; struct TestData { @@ -37,9 +40,8 @@ struct TestData va_end(ap); } - void AddResult(uint16_t pos, uint16_t len) { m_results.push_back(TestResult(pos, len)); } + void AddResult(uint16_t pos, uint16_t len) { m_results.emplace_back(pos, len); } }; -typedef vector TestVector; class CheckRange { @@ -86,7 +88,7 @@ UNIT_TEST(SearchStringTokensIntersectionRange) char const * lowTokens8[] = {"ул", "кар"}; char const * lowTokens9[] = {"ул", "бог"}; - TestVector tests; + vector tests; // fill test data tests.push_back(TestData(str0, lowTokens0, 2, 2, 6, 5, 12, 6)); tests.push_back(TestData(str1, lowTokens0, 2, 2, 4, 5, 10, 6)); @@ -112,12 +114,8 @@ UNIT_TEST(SearchStringTokensIntersectionRange) tests.push_back(TestData(str9, lowTokens8, 2, 2, 0, 2, 6, 3)); tests.push_back(TestData(str11, lowTokens9, 2, 2, 0, 2, 14, 3)); - // run tests - size_t count = 0; - for (TestVector::iterator it = tests.begin(); it != tests.end(); ++it, ++count) + for (TestData const & data : tests) { - TestData const & data = *it; - search::SearchStringTokensIntersectionRanges( data.m_input, data.m_lowTokens.begin(), data.m_lowTokens.end(), CheckRange(data.m_results)); } diff --git a/search/suggest.cpp b/search/suggest.cpp index a26ca57342..5272337d66 100644 --- a/search/suggest.cpp +++ b/search/suggest.cpp @@ -47,7 +47,7 @@ void GetSuggestion(RankerResult const & res, string const & query, QueryTokens c if (!prefixMatched || fullPrefixMatched) return; - GetStringPrefix(query, suggest); + suggest = DropLastToken(query); // Appends unmatched result's tokens to the suggestion. for (size_t i = 0; i < tokens.size(); ++i)