refactor: Make Join() util work with any container type #25879

MarcoFalke commented at 5:55 PM on August 19, 2022: member

This allows to drop some code

Remove Join() helper only used in tests

Also remove redundant return type that can be deduced by the compiler.

faf8da3c8d

DrahtBot added the label Refactoring on Aug 19, 2022

NorrinRadd commented at 2:34 AM on August 22, 2022: none

LGTM

in src/util/string.h:61 in fa75227e82 outdated

  57 | @@ -58,34 +58,29 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin
  58 |  }
  59 |  
  60 |  /**
  61 | - * Join a list of items
  62 | + * Join items

stickies-v commented at 7:19 PM on August 22, 2022:

nit: perhaps a bit more info:

 * Join all collection items into an object with the same type as the first item, separated by a separator

MarcoFalke commented at 5:59 AM on August 23, 2022:

That'd be incorrect. It isn't the type of the first item, but the type returned by the unary op executed on the first item. In practise this will (almost) always be std::string, so it seems odd to be overly verbose?

stickies-v commented at 8:10 AM on August 23, 2022:

Okay, no strong feelings about it. For a helper function that's meant to be used all over the codebase I just think it's worth thinking more about a helpful docstring, maybe a join function is not be trivial to everyone? Final suggestion but I don't want to waste time over this so feel free to ignore if you don't think it's helpful:

 * Join all container items. Typically used to concatenate strings but accepts containers with
 * elements of any type.

MarcoFalke commented at 8:14 AM on August 24, 2022:

thx, done

in src/util/string.h:76 in fa75227e82 outdated

  80 | +    decltype(unary_op(*collection.begin())) ret;
  81 | +    bool first{true};
  82 | +    for (const auto& item : collection) {
  83 | +        if (!first) ret += separator;
  84 | +        ret += unary_op(item);
  85 | +        first = false;

stickies-v commented at 7:45 PM on August 22, 2022:

Could potentially do away with first:

    for (const auto& item : collection) {
        if (&item != &(*collection.begin())) ret += separator;
        ret += unary_op(item);
    }

MarcoFalke commented at 6:05 AM on August 23, 2022:

That wouldn't compile for all types. For example, https://en.cppreference.com/w/cpp/container/vector_bool/reference has no operator&

stickies-v commented at 7:41 AM on August 23, 2022:

I see, thanks for the info. Didn't know about this, that's annoying!

in src/util/string.h:63 in fa75227e82 outdated

  57 | @@ -58,34 +58,29 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin
  58 |  }
  59 |  
  60 |  /**
  61 | - * Join a list of items
  62 | + * Join items
  63 |   *
  64 | - * @param list       The list to join
  65 | + * @param collection The items to join

stickies-v commented at 7:53 PM on August 22, 2022:

nit: container seems to be the term used in c++, would it be better to be consistent with that?

MarcoFalke commented at 6:06 AM on August 23, 2022:

Yes, I think I got collection from python https://docs.python.org/3/library/collections.html

in src/util/string.h:81 in fa75227e82 outdated

  88 |  }
  89 |  
  90 | -template <typename T, typename T2>
  91 | -T Join(const std::vector<T>& list, const T2& separator)
  92 | +template <typename C, typename S>
  93 | +auto Join(const C& list, const S& separator)

stickies-v commented at 8:07 PM on August 22, 2022:

nit: parameter name inconsistent

auto Join(const C& collection, const S& separator)

MarcoFalke commented at 6:14 AM on August 23, 2022:

thx, done

stickies-v commented at 8:36 PM on August 22, 2022: contributor

Approach ACK fa75227e82876ba944993d7b175586616a1cf5d4

MarcoFalke force-pushed on Aug 23, 2022

naumenkogs commented at 8:02 AM on August 24, 2022: member

utACK fabedbc0c24039da62102c4c0a161c2a643a6b88

btw, do you wanna update PR name and commit messages to use container instead of collection too?

MarcoFalke renamed this:
~~refactor: Make Join() util work with any collection type~~
refactor: Make Join() util work with any container type
on Aug 24, 2022

Make Join() util work with any container type

Also, remove helper that is only used in tests.

fa1c716955

Use new Join() helper for ListBlockFilterTypes() fa95315655

MarcoFalke commented at 8:18 AM on August 24, 2022: member

thx, renamed title

MarcoFalke force-pushed on Aug 24, 2022

stickies-v approved

stickies-v commented at 8:56 AM on August 24, 2022: contributor

ACK fa95315

naumenkogs commented at 9:18 AM on August 24, 2022: member

ACK fa95315655fcd31a5482f5313faf04dbfa4de580

MarcoFalke merged this on Aug 24, 2022

MarcoFalke closed this on Aug 24, 2022

MarcoFalke deleted the branch on Aug 24, 2022

sidhujag referenced this in commit 5d00eaee3b on Aug 24, 2022

bitcoin locked this on Aug 24, 2023