Merge pull request #24426 from vespa-engine/balder/cleanup-sorting-code-cpp

Various cleanup while reading sorting code in backend.

3 files changed, 35 insertions, 57 deletions

diff --git a/searchlib/src/vespa/searchlib/common/sortresults.cpp b/searchlib/src/vespa/searchlib/common/sortresults.cpp
index 1add3501f61..911e447715b 100644
--- a/searchlib/src/vespa/searchlib/common/sortresults.cpp
+++ b/searchlib/src/vespa/searchlib/common/sortresults.cpp
@@ -34,28 +34,25 @@ public:
     }
 };
 
-} // namespace <unnamed>
-
-
-inline void
-FastS_insertion_sort(RankedHit a[], uint32_t n)
-{
+void
+insertion_sort(RankedHit a[], uint32_t n) {
     uint32_t i, j;
     RankedHit swap;
     typedef RadixHelper<search::HitRank> RT;
     RT R;
 
-    for (i=1; i<n ; i++) {
+    for (i = 1; i < n; i++) {
         swap = a[i];
         j = i;
-        while (R(swap.getRank()) > R(a[j-1].getRank())) {
-            a[j] = a[j-1];
-            if (!(--j)) break;;
+        while (R(swap.getRank()) > R(a[j - 1].getRank())) {
+            a[j] = a[j - 1];
+            if (!(--j)) break;
         }
         a[j] = swap;
     }
 }
 
+}
 
 template<int SHIFT>
 void
@@ -133,14 +130,12 @@ FastS_radixsort(RankedHit a[], uint32_t n, uint32_t ntop)
             if ((last[i]-cnt[i])<ntop) {
                 if (cnt[i]>INSERT_SORT_LEVEL) {
                     if (last[i]<ntop) {
-                        FastS_radixsort<SHIFT - 8>(&a[last[i]-cnt[i]], cnt[i],
-                                cnt[i]);
+                        FastS_radixsort<SHIFT - 8>(&a[last[i]-cnt[i]], cnt[i], cnt[i]);
                     } else {
-                        FastS_radixsort<SHIFT - 8>(&a[last[i]-cnt[i]], cnt[i],
-                                cnt[i]+ntop-last[i]);
+                        FastS_radixsort<SHIFT - 8>(&a[last[i]-cnt[i]], cnt[i], cnt[i]+ntop-last[i]);
                     }
                 } else if (cnt[i]>1) {
-                        FastS_insertion_sort(&a[last[i]-cnt[i]], cnt[i]);
+                    insertion_sort(&a[last[i]-cnt[i]], cnt[i]);
                 }
             }
         }
@@ -156,13 +151,13 @@ FastS_SortResults(RankedHit a[], uint32_t n, uint32_t ntop)
     if (n > INSERT_SORT_LEVEL) {
         FastS_radixsort<sizeof(search::HitRank)*8 - 8>(a, n, ntop);
     } else {
-        FastS_insertion_sort(a, n);
+        insertion_sort(a, n);
     }
 }
 
 //-----------------------------------------------------------------------------
 
-FastS_DefaultResultSorter FastS_DefaultResultSorter::__instance;
+FastS_DefaultResultSorter FastS_DefaultResultSorter::_instance;
 
 //-----------------------------------------------------------------------------
 
@@ -173,7 +168,7 @@ FastS_SortSpec::Add(IAttributeContext & vecMan, const SortInfo & sInfo)
         return false;
 
     uint32_t          type   = ASC_VECTOR;
-    const IAttributeVector * vector(NULL);
+    const IAttributeVector * vector(nullptr);
 
     if ((sInfo._field.size() == 6) && (sInfo._field == "[rank]")) {
         type = (sInfo._ascending) ? ASC_RANK : DESC_RANK;
@@ -220,16 +215,16 @@ FastS_SortSpec::initSortData(const RankedHit *hits, uint32_t n)
     freeSortData();
     size_t fixedWidth = 0;
     size_t variableWidth = 0;
-    for (auto iter = _vectors.begin(); iter != _vectors.end(); ++iter) {
-        if (iter->_type >= ASC_DOCID) { // doc id
+    for (const auto & vec : _vectors) {
+        if (vec._type >= ASC_DOCID) { // doc id
             fixedWidth += sizeof(uint32_t) + sizeof(uint16_t);
-        }else if (iter->_type >= ASC_RANK) { // rank value
+        } else if (vec._type >= ASC_RANK) { // rank value
             fixedWidth += sizeof(search::HitRank);
         } else {
-            size_t numBytes = iter->_vector->getFixedWidth();
+            size_t numBytes = vec._vector->getFixedWidth();
             if (numBytes == 0) { // string
                 variableWidth += 11;
-            } else if (!iter->_vector->hasMultiValue()) {
+            } else if (!vec._vector->hasMultiValue()) {
                 fixedWidth += numBytes;
             }
         }
@@ -243,13 +238,13 @@ FastS_SortSpec::initSortData(const RankedHit *hits, uint32_t n)
 
     for (uint32_t i(0), idx(0); (i < n) && !_doom.hard_doom(); ++i) {
         uint32_t len = 0;
-        for (auto iter = _vectors.begin(); iter != _vectors.end(); ++iter) {
+        for (const auto & vec : _vectors) {
             int written(0);
             if (available < std::max(sizeof(hits->_docId) + sizeof(_partitionId), sizeof(hits->_rankValue))) {
                 mySortData = realloc(n, variableWidth, available, dataSize, mySortData);
             }
             do {
-                switch (iter->_type) {
+                switch (vec._type) {
                 case ASC_DOCID:
                     serializeForSort<convertForSort<uint32_t, true> >(hits[i].getDocId(), mySortData);
                     serializeForSort<convertForSort<uint16_t, true> >(_partitionId, mySortData + sizeof(hits->_docId));
@@ -269,10 +264,10 @@ FastS_SortSpec::initSortData(const RankedHit *hits, uint32_t n)
                     written = sizeof(hits->_rankValue);
                     break;
                 case ASC_VECTOR:
-                    written = iter->_vector->serializeForAscendingSort(hits[i].getDocId(), mySortData, available, iter->_converter);
+                    written = vec._vector->serializeForAscendingSort(hits[i].getDocId(), mySortData, available, vec._converter);
                     break;
                 case DESC_VECTOR:
-                    written = iter->_vector->serializeForDescendingSort(hits[i].getDocId(), mySortData, available, iter->_converter);
+                    written = vec._vector->serializeForDescendingSort(hits[i].getDocId(), mySortData, available, vec._converter);
                     break;
                 }
                 if (written == -1) {
@@ -293,7 +288,6 @@ FastS_SortSpec::initSortData(const RankedHit *hits, uint32_t n)
     }
 }
 
-
 FastS_SortSpec::FastS_SortSpec(uint32_t partitionId, const Doom & doom, const ConverterFactory & ucaFactory) :
     _partitionId(partitionId),
     _doom(doom),
@@ -308,7 +302,6 @@ FastS_SortSpec::~FastS_SortSpec()
     freeSortData();
 }
 
-
 bool
 FastS_SortSpec::Init(const string & sortStr, IAttributeContext & vecMan)
 {
@@ -316,7 +309,7 @@ FastS_SortSpec::Init(const string & sortStr, IAttributeContext & vecMan)
     bool retval(true);
     try {
         _sortSpec = SortSpec(sortStr, _ucaFactory);
-        for (SortSpec::const_iterator it(_sortSpec.begin()), mt(_sortSpec.end()); retval && (it < mt); it++) {
+        for (auto it(_sortSpec.begin()); retval && (it != _sortSpec.end()); it++) {
             retval = Add(vecMan, *it);
         }
     } catch (const std::exception & e) {
@@ -374,26 +367,11 @@ FastS_SortSpec::initWithoutSorting(const RankedHit * hits, uint32_t hitCnt)
     initSortData(hits, hitCnt);
 }
 
-inline int
-FastS_SortSpec::Compare(const FastS_SortSpec *self, const SortData &a,
-                        const SortData &b)
-{
-    const uint8_t * ref = self->_binarySortData.data();
-    uint32_t len = a._len < b._len ? a._len : b._len;
-    int retval = memcmp(ref + a._idx,
-                        ref + b._idx, len);
-    if (retval < 0) {
-        return -1;
-    } else if (retval > 0) {
-        return 1;
-    }
-    return 0;
-}
 
 class StdSortDataCompare
 {
 public:
-    StdSortDataCompare(const uint8_t * s) : _sortSpec(s) { }
+    explicit StdSortDataCompare(const uint8_t * s) : _sortSpec(s) { }
     bool operator() (const FastS_SortSpec::SortData & x, const FastS_SortSpec::SortData & y) const {
         return cmp(x, y) < 0;
     }
@@ -409,7 +387,7 @@ private:
 class SortDataRadix
 {
 public:
-    SortDataRadix(const uint8_t * s) : _data(s) { }
+    explicit SortDataRadix(const uint8_t * s) : _data(s) { }
     uint32_t operator () (FastS_SortSpec::SortData & a) const {
         uint32_t r(0);
         uint32_t left(a._len - a._pos);
@@ -448,10 +426,11 @@ void
 FastS_SortSpec::sortResults(RankedHit a[], uint32_t n, uint32_t topn)
 {
     initSortData(a, n);
-    SortData * sortData = _sortDataArray.data();
     {
+        SortData * sortData = _sortDataArray.data();
+        const uint8_t * binary = _binarySortData.data();
         Array<uint32_t> radixScratchPad(n, Alloc::alloc(0, MMAP_LIMIT));
-        search::radix_sort(SortDataRadix(_binarySortData.data()), StdSortDataCompare(_binarySortData.data()), SortDataEof(), 1, sortData, n, radixScratchPad.data(), 0, 96, topn);
+        search::radix_sort(SortDataRadix(binary), StdSortDataCompare(binary), SortDataEof(), 1, sortData, n, radixScratchPad.data(), 0, 96, topn);
     }
     for (uint32_t i(0), m(_sortDataArray.size()); i < m; ++i) {
         a[i]._rankValue = _sortDataArray[i]._rankValue;
diff --git a/searchlib/src/vespa/searchlib/common/sortresults.h b/searchlib/src/vespa/searchlib/common/sortresults.h
index 3e64aca269d..9ea7d4f14ba 100644
--- a/searchlib/src/vespa/searchlib/common/sortresults.h
+++ b/searchlib/src/vespa/searchlib/common/sortresults.h
@@ -28,7 +28,7 @@ struct FastS_IResultSorter {
     /**
      * Destructor.  No cleanup needed for base class.
      */
-    virtual ~FastS_IResultSorter() {}
+    virtual ~FastS_IResultSorter() = default;
 
     /**
      * Sort the given array of results.
@@ -45,10 +45,10 @@ struct FastS_IResultSorter {
 class FastS_DefaultResultSorter : public FastS_IResultSorter
 {
 private:
-    static FastS_DefaultResultSorter __instance;
+    static FastS_DefaultResultSorter _instance;
 
 public:
-    static FastS_DefaultResultSorter *instance() { return &__instance; }
+    static FastS_DefaultResultSorter *instance() { return &_instance; }
     void sortResults(search::RankedHit a[], uint32_t n, uint32_t ntop) override {
         return FastS_SortResults(a, n, ntop);
     }
@@ -84,6 +84,7 @@ public:
 
     struct SortData : public search::RankedHit
     {
+        SortData() : RankedHit(), _idx(0u), _len(0u), _pos(0u) {}
         uint32_t _idx;
         uint32_t _len;
         uint32_t _pos;
@@ -110,11 +111,10 @@ public:
     FastS_SortSpec(const FastS_SortSpec &) = delete;
     FastS_SortSpec & operator = (const FastS_SortSpec &) = delete;
     FastS_SortSpec(uint32_t partitionId, const vespalib::Doom & doom, const ConverterFactory & ucaFactory);
-    ~FastS_SortSpec();
+    ~FastS_SortSpec() override;
 
     std::pair<const char *, size_t> getSortRef(size_t i) const {
-        return std::pair<const char *, size_t>((const char*)(&_binarySortData[0] + _sortDataArray[i]._idx),
-                                               _sortDataArray[i]._len);
+        return {(const char*)(&_binarySortData[0] + _sortDataArray[i]._idx), _sortDataArray[i]._len };
     }
     bool Init(const vespalib::string & sortSpec, search::attribute::IAttributeContext & vecMan);
     void sortResults(search::RankedHit a[], uint32_t n, uint32_t topn) override;
@@ -122,7 +122,6 @@ public:
     void copySortData(uint32_t offset, uint32_t n, uint32_t *idx, char *buf);
     void freeSortData();
     void initWithoutSorting(const search::RankedHit * hits, uint32_t hitCnt);
-    static int Compare(const FastS_SortSpec *self, const SortData &a, const SortData &b);
 };
 
 //-----------------------------------------------------------------------------
diff --git a/searchlib/src/vespa/searchlib/common/threaded_compactable_lid_space.cpp b/searchlib/src/vespa/searchlib/common/threaded_compactable_lid_space.cpp
index cd4069ac959..80f5f1ac5e8 100644
--- a/searchlib/src/vespa/searchlib/common/threaded_compactable_lid_space.cpp
+++ b/searchlib/src/vespa/searchlib/common/threaded_compactable_lid_space.cpp
@@ -8,7 +8,7 @@ namespace search::common {
 ThreadedCompactableLidSpace::ThreadedCompactableLidSpace(std::shared_ptr<ICompactableLidSpace> target,
                                                          ISequencedTaskExecutor &executor,
                                                          ISequencedTaskExecutor::ExecutorId id)
-    : _target(target),
+    : _target(std::move(target)),
       _executor(executor),
       _executorId(id)
 {