refactor: Make Join() util work with any container type #25879
Conversation
Also remove redundant return type that can be deduced by the compiler.
|
LGTM |
src/util/string.h
Outdated
| @@ -58,34 +58,29 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin | |||
| } | |||
|
|
|||
| /** | |||
| * Join a list of items | |||
| * Join items | |||
There was a problem hiding this comment.
nit: perhaps a bit more info:
| * Join items | |
| * Join all collection items into an object with the same type as the first item, separated by a separator |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 items | |
| * Join all container items. Typically used to concatenate strings but accepts containers with | |
| * elements of any type. |
src/util/string.h
Outdated
| bool first{true}; | ||
| for (const auto& item : collection) { | ||
| if (!first) ret += separator; | ||
| ret += unary_op(item); | ||
| first = false; |
There was a problem hiding this comment.
Could potentially do away with first:
| bool first{true}; | |
| for (const auto& item : collection) { | |
| if (!first) ret += separator; | |
| ret += unary_op(item); | |
| first = false; | |
| for (const auto& item : collection) { | |
| if (&item != &(*collection.begin())) ret += separator; | |
| ret += unary_op(item); | |
| } |
There was a problem hiding this comment.
That wouldn't compile for all types. For example, https://en.cppreference.com/w/cpp/container/vector_bool/reference has no operator&
There was a problem hiding this comment.
I see, thanks for the info. Didn't know about this, that's annoying!
src/util/string.h
Outdated
| * | ||
| * @param list The list to join | ||
| * @param collection The items to join |
There was a problem hiding this comment.
nit: container seems to be the term used in c++, would it be better to be consistent with that?
There was a problem hiding this comment.
Yes, I think I got collection from python https://docs.python.org/3/library/collections.html
src/util/string.h
Outdated
| template <typename T, typename T2> | ||
| T Join(const std::vector<T>& list, const T2& separator) | ||
| template <typename C, typename S> | ||
| auto Join(const C& list, const S& separator) |
There was a problem hiding this comment.
nit: parameter name inconsistent
| auto Join(const C& list, const S& separator) | |
| auto Join(const C& collection, const S& separator) |
|
utACK fabedbc btw, do you wanna update PR name and commit messages to use |
maflcko
changed the title
Also, remove helper that is only used in tests.
|
thx, renamed title |
|
ACK fa95315 |
…er type fa95315 Use new Join() helper for ListBlockFilterTypes() (MacroFake) fa1c716 Make Join() util work with any container type (MacroFake) faf8da3 Remove Join() helper only used in tests (MacroFake) Pull request description: This allows to drop some code ACKs for top commit: naumenkogs: ACK fa95315 stickies-v: ACK [fa95315](bitcoin@fa95315) Tree-SHA512: efd65b65722f46b221bd53140ff22bd8e45adc83617980233f28f695be3108a6ab01affd751d715134ffcb9762228ba8952e9467e590cff022c83e0f5404cb74
This allows to drop some code