On Jul 12, 2013, at 5:42 PM, Marshall Clow <[email protected]> wrote:
> On Jul 12, 2013, at 12:42 PM, Howard Hinnant <[email protected]> wrote: > >> On Jul 11, 2013, at 12:29 PM, Marshall Clow <[email protected]> wrote: >> >>> Tuple and pair. >> >> This is really nice, thanks Marshall! I especially like the care you put >> into the diagnostic messages. > > Thanks! > > >> A few minor comments: >> >> 1. The new tests should go under tuple.tuple/tuple.elem, instead of under >> tuple.general, because the former is where the standard defines them. > > Done. > >> >> 2. This technically doesn't matter (else the tests wouldn't be passing), >> but I would prefer that the new get definitions go /after/ the old get >> definitions in <tuple> since the new one's call the old ones. The first >> time I looked at this I had to ask myself if qualified name lookup would >> really find the old get's from inside the new get's. I'd rather code >> readers not need to ask this question. > > Done. > >> 3. In tuple.by.type.pass.cpp, in two places make_tuple is used with >> explicit template arguments. I wold prefer instead that make_tuple not be >> used at all: >> >> std::tuple<int, std::string, cf> t1( 42, "Hi", { 1,2 } ); >> ... >> std::tuple<int, std::string, int, cf> t2( 42, "Hi", 23, { 1,2 } ); >> >> This is just less complicated. > > Done. > >> 4. Also in tuple.by.type.pass.cpp, I'd like to see this test added: >> >> { >> std::tuple<std::unique_ptr<int>> t(std::unique_ptr<int>(new int(4))); >> std::unique_ptr<int> p = std::get<std::unique_ptr<int>>(std::move(t)); >> assert(*p == 4); >> assert(std::get<0>(t) == nullptr); >> } >> >> This test should reveal a minor bug in the implementation (you've already >> fixed it in pair :-)). As a sanity check, it probably wouldn't hurt to make >> a .fail.cpp out of this too: > > Done and fixed. > >> { >> std::tuple<std::unique_ptr<int>> t(std::unique_ptr<int>(new int(4))); >> std::unique_ptr<int> p = std::get<std::unique_ptr<int>>(t); >> } > > Done. > >> 5. I don't see any tests for get<T>(pair). > > They didn't get included in the diff :-( > They're there now. > > Thanks for the review. This looks great to me Marshall! Thanks much for your perseverance on this. The only nit I have is that get<type>(tuple&&) should use move instead of forward (you've already got this correct in get<type>(pair&&)). Please commit with this minor change. Thanks, Howard _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
