From 0a75ca7960ef42801d24b3605594701a45aca3a0 Mon Sep 17 00:00:00 2001
From: "Ralph J. Steinhagen" <r.steinhagen@gsi.de>
Date: Wed, 25 Dec 2024 17:49:03 +0100
Subject: [PATCH] Selector block: fix sign-conversion warnings & minor cleanup

- explicitly cast sizes for std::copy_n and tag checks
- use single-step wrap for negative tag index checks

Signed-off-by: Ralph J. Steinhagen <r.steinhagen@gsi.de>
---
 .../include/gnuradio-4.0/basic/Selector.hpp   | 45 ++++++++++---------
 1 file changed, 24 insertions(+), 21 deletions(-)

diff --git a/blocks/basic/include/gnuradio-4.0/basic/Selector.hpp b/blocks/basic/include/gnuradio-4.0/basic/Selector.hpp
index bdddde4c..d956ac0d 100644
--- a/blocks/basic/include/gnuradio-4.0/basic/Selector.hpp
+++ b/blocks/basic/include/gnuradio-4.0/basic/Selector.hpp
@@ -11,8 +11,7 @@ using namespace gr;
 
 template<typename T>
 struct Selector : Block<Selector<T>, NoDefaultTagForwarding> {
-    using Description = Doc<R""(
-@brief basic multiplexing class to route arbitrary inputs to outputs
+    using Description = Doc<R""(@brief basic multiplexing class to route arbitrary inputs to outputs
 
 See https://wiki.gnuradio.org/index.php/Selector
 
@@ -114,22 +113,22 @@ you can set the `backPressure` property to false.
             }
 
             std::set<std::pair<gr::Size_t, gr::Size_t>> duplicateSet{};
+            for (auto i : std::views::iota(static_cast<std::size_t>(0), map_in.value.size())) {
+                gr::Size_t inIdx  = map_in.value[i];
+                gr::Size_t outIdx = map_out.value[i];
 
-            for (std::size_t i = 0U; i < map_out.value.size(); ++i) {
-                _internalMappingInOut[static_cast<std::size_t>(map_in.value[i])].push_back(static_cast<std::size_t>(map_out.value[i]));
-                _internalMappingOutIn[static_cast<std::size_t>(map_out.value[i])].push_back(static_cast<std::size_t>(map_in.value[i]));
+                _internalMappingInOut[inIdx].push_back(outIdx);
+                _internalMappingOutIn[outIdx].push_back(inIdx);
 
-                const auto isDuplicate = !duplicateSet.insert({map_in.value[i], map_out.value[i]}).second;
-                if (isDuplicate) {
-                    throw std::invalid_argument(fmt::format("map_in[{}]:{} and map_out[{}]:{} are duplicated", i, map_in.value[i], i, map_out.value[i]));
+                if (!duplicateSet.insert({inIdx, outIdx}).second) { // check for duplicates
+                    throw std::invalid_argument(fmt::format("Duplicate pair (in:{}, out:{}) at i={}", inIdx, outIdx, i));
                 }
 
-                if (map_in.value[i] >= n_inputs) {
-                    throw std::invalid_argument(fmt::format("map_in[{}] contains port index ({}) > n_inputs ({})", i, map_in.value[i], n_inputs));
+                if (inIdx >= n_inputs) { // range checks
+                    throw std::invalid_argument(fmt::format("map_in[{}] = {} is >= n_inputs ({})", i, inIdx, n_inputs));
                 }
-
-                if (map_out.value[i] >= n_outputs) {
-                    throw std::invalid_argument(fmt::format("map_out[{}] contains port index ({}) > n_outputs ({})", i, map_in.value[i], n_outputs));
+                if (outIdx >= n_outputs) {
+                    throw std::invalid_argument(fmt::format("map_out[{}] = {} is >= n_outputs ({})", i, outIdx, n_outputs));
                 }
             }
         }
@@ -151,15 +150,19 @@ you can set the `backPressure` property to false.
 
         std::vector<std::size_t> outOffsets(outs.size(), 0UZ);
         auto                     copyToOutput = [&outOffsets](std::size_t nSamplesToCopy, auto& inputSpan, auto& outputSpan, std::size_t outIndex) {
-            const std::size_t offset = outIndex == std::numeric_limits<std::size_t>::max() ? 0UZ : outOffsets[outIndex];
-            std::copy_n(inputSpan.begin(), nSamplesToCopy, std::next(outputSpan.begin(), offset));
+            const auto diffCount  = static_cast<std::ptrdiff_t>(nSamplesToCopy);
+            const auto diffOffset = static_cast<std::ptrdiff_t>((outIndex == std::numeric_limits<std::size_t>::max()) ? 0U : outOffsets[outIndex]);
+
+            std::copy_n(inputSpan.begin(), diffCount, std::next(outputSpan.begin(), diffOffset));
+
             if (outIndex != std::numeric_limits<std::size_t>::max()) {
                 outOffsets[outIndex] += nSamplesToCopy;
             }
-            const auto tags = inputSpan.tags();
-            for (const auto& tag : tags) {
-                if (tag.first < nSamplesToCopy) {
-                    outputSpan.publishTag(tag.second, tag.first + offset);
+
+            const auto tags = inputSpan.tags(); // Tag handling
+            for (const auto& [normalisedTagIndex, tagMap] : tags) {
+                if (normalisedTagIndex < static_cast<std::ptrdiff_t>(nSamplesToCopy)) {
+                    outputSpan.publishTag(tagMap, std::max(static_cast<std::size_t>(normalisedTagIndex) + static_cast<std::size_t>(diffOffset), 0UZ));
                 }
             }
             outputSpan.publish(nSamplesToCopy);
@@ -205,7 +208,7 @@ you can set the `backPressure` property to false.
                             outSpan[nSamplesToPublish] = ins[inIndex][iS];
                             for (const auto& tag : ins[inIndex].rawTags) {
                                 const auto relIndex = tag.index >= ins[inIndex].streamIndex ? static_cast<std::ptrdiff_t>(tag.index - ins[inIndex].streamIndex) : -static_cast<std::ptrdiff_t>(ins[inIndex].streamIndex - tag.index);
-                                if (relIndex == iS) {
+                                if (relIndex == static_cast<std::ptrdiff_t>(iS)) {
                                     outSpan.publishTag(tag.map, nSamplesToPublish);
                                 }
                             }
@@ -247,7 +250,7 @@ you can set the `backPressure` property to false.
 };
 } // namespace gr::basic
 
-auto registerSelector = gr::registerBlock<gr::basic::Selector, float, double>(gr::globalBlockRegistry());
+inline static auto registerSelector = gr::registerBlock<gr::basic::Selector, float, double>(gr::globalBlockRegistry());
 static_assert(gr::HasProcessBulkFunction<gr::basic::Selector<double>>);
 
 #endif // include guard