From 1df77cc4ffe84d0b4910a07b224770dfdc9042dc Mon Sep 17 00:00:00 2001 From: Ichthyostega Date: Sat, 2 Dec 2017 04:06:11 +0100 Subject: [PATCH] Library: investigate and fix an insidious problem with move-forwarding (util::join / transformIter) As it turned out, we had two bugs luring in the code base, with the happy result of one cancelling out the adverse effects of the other :-D - a mistake in the invocation of the Itertools (transform, filter,...) caused them to move and consume any input passed by forwarding, instead of consuming only the RValue references. - but util::join did an extraneous copy on its data source, meaning that in all relevant cases where a *copy* got passed into the Itertools, only that spurious temporary was consumed by Bug #1. (Note that most usages of Itertools rely on RValues anyway, since the whole point of Itertools is to write concise in-line transformation pipelines...) *** Added additional testcode to prove util::stringify() behaves correct now in all cases. --- doc/technical/code/c++11.txt | 6 +- src/lib/format-util.hpp | 3 +- src/lib/itertools.hpp | 6 +- tests/library/format-helper-test.cpp | 25 ++++++++ wiki/thinkPad.ichthyo.mm | 88 ++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 5 deletions(-) diff --git a/doc/technical/code/c++11.txt b/doc/technical/code/c++11.txt index 850f9f4b9..dad754c0e 100644 --- a/doc/technical/code/c++11.txt +++ b/doc/technical/code/c++11.txt @@ -163,7 +163,11 @@ of arbitrary type to the constructor of type `TY`. Note the following details that a rvalue reference can only bind to _something unnamed_. This is a safety feature; moving destroys the contents at the source location. Thus if we really want to move something known by name (like e.g. the function arguments in this example), we need to indicate so by an explicit call. -- `std::forward` needs an explicit type parameter to be able to deduce the right target type in any case +- `std::forward` needs an explicit type parameter to be able to deduce the right target type in any case. + It is _crucial to pass this type parameter in the correct way,_ as demonstrated above. Because, if we + invoke this function for _copying_ (i.e. with a lvalue reference), this type parameter will _carry on_ + this refrerence. Only a rvalue reference is stripped. This is the only way ``downstream'' receivers can + tell the copy or move case apart. - note how we're allowed to ``wrap'' a function call around the unpacking of the _argument pack_ (`args`): the three dots `...` are _outside_ the parenthesis, and thus the `std::forward` is applied to each of the arguments individually diff --git a/src/lib/format-util.hpp b/src/lib/format-util.hpp index 56a68ebcc..08700b8f6 100644 --- a/src/lib/format-util.hpp +++ b/src/lib/format-util.hpp @@ -110,6 +110,7 @@ namespace util { /** convert a sequence of elements to string * @param elms sequence of arbitrary elements + * @tparam CON the container type to collect the results * @return a collection of type CON, initialised by the * string representation of the given elements */ @@ -192,7 +193,7 @@ namespace util { using Coll = typename lib::meta::Strip::TypePlain; _RangeIter range(std::forward(coll)); - auto strings = stringify (range.iter); + auto strings = stringify (std::move (range.iter)); if (!strings) return ""; std::ostringstream buffer; diff --git a/src/lib/itertools.hpp b/src/lib/itertools.hpp index 86a2e6705..e95fdd654 100644 --- a/src/lib/itertools.hpp +++ b/src/lib/itertools.hpp @@ -383,7 +383,7 @@ namespace lib { filterIterator (IT&& src, PRED filterPredicate) { using SrcIT = typename std::remove_reference::type; - return FilterIter{forward(src), filterPredicate}; + return FilterIter{forward(src), filterPredicate}; } @@ -799,7 +799,7 @@ namespace lib { { using SrcIT = typename std::remove_reference::type; using OutVal = typename lib::meta::_Fun::Ret; - return TransformIter{forward(src), processingFunc}; + return TransformIter{forward(src), processingFunc}; } @@ -857,7 +857,7 @@ namespace lib { { using SrcIT = typename std::remove_reference::type; using Val = typename SrcIT::value_type; - return filterIterator (forward(source), SkipRepetition() ); + return filterIterator (forward(source), SkipRepetition() ); } diff --git a/tests/library/format-helper-test.cpp b/tests/library/format-helper-test.cpp index eb1c46f5e..be22577ea 100644 --- a/tests/library/format-helper-test.cpp +++ b/tests/library/format-helper-test.cpp @@ -36,6 +36,7 @@ using lib::transformIterator; +using lib::iter_stl::snapshot; using lib::iter_stl::eachElm; using lib::eachNum; using util::_Fmt; @@ -157,6 +158,30 @@ namespace test { // another variant: collect arbitrary number of arguments VecS vals = stringify (short(12), 345L, "67", '8'); CHECK (vals == VecS({"12", "345", "67", "8"})); + + + // stringify can both consume (RValue-Ref) or take a copy from its source + auto nn = snapshot (eachNum (5, 10)); + CHECK (5 == *nn); + ++nn; + CHECK (6 == *nn); + + auto sn = stringify (nn); + CHECK ("6" == *sn); + ++sn; + CHECK ("7" == *sn); + CHECK (6 == *nn); + ++++nn; + CHECK (8 == *nn); + CHECK ("7" == *sn); + + sn = stringify (std::move(nn)); + CHECK ("8" == *sn); + CHECK (isnil (nn)); // was consumed by moving it into sn + ++sn; + CHECK ("9" == *sn); + ++sn; + CHECK (isnil (sn)); } diff --git a/wiki/thinkPad.ichthyo.mm b/wiki/thinkPad.ichthyo.mm index 046d71af2..ea207732a 100644 --- a/wiki/thinkPad.ichthyo.mm +++ b/wiki/thinkPad.ichthyo.mm @@ -5419,6 +5419,22 @@ + + + + + + + + + + + + + + + + @@ -5530,6 +5546,78 @@ + + + + + + + + + + + + + + + + + + + +

+ versehentlich wurde auch der an std::forward gegeben +

+ + + + + + + + + + +

+ habs mit FormatUtils_test bewiesen +

+

+ Dazu in NumIter einen explizit tracenden move-ctor eingebaut +

+ + + + + + + + + + + +

+ weil der Aufruf von join(&&) selber wasserdicht ist +

+

+ D.h. er frisst keine Werte. +

+

+ Deshalb fällt dieses doppelte Problem nicht auf +

+ + + + + + + + + + + + + +