From reviews-return-19774-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Mon Jan 11 12:34:37 2016 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 5CDF918C01 for ; Mon, 11 Jan 2016 12:34:37 +0000 (UTC) Received: (qmail 41920 invoked by uid 500); 11 Jan 2016 12:34:37 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 41897 invoked by uid 500); 11 Jan 2016 12:34:37 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 41880 invoked by uid 99); 11 Jan 2016 12:34:37 -0000 Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 11 Jan 2016 12:34:37 +0000 Received: from reviews.apache.org (localhost [127.0.0.1]) by reviews.apache.org (Postfix) with ESMTP id 78B7C1D2740; Mon, 11 Jan 2016 12:34:36 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============9010457190405382552==" MIME-Version: 1.0 Subject: Re: Review Request 41875: Provided List constructor from std::initializer_list. From: "Alexander Rojas" To: "Till Toenshoff" , "Ben Mahler" Cc: "Alexander Rojas" , "Benjamin Bannier" , "mesos" Date: Mon, 11 Jan 2016 12:34:36 -0000 Message-ID: <20160111123436.26792.73329@reviews.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: "Alexander Rojas" X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/41875/ X-Sender: "Alexander Rojas" References: <20160108113853.1692.88399@reviews.apache.org> In-Reply-To: <20160108113853.1692.88399@reviews.apache.org> Reply-To: "Alexander Rojas" X-ReviewRequest-Repository: mesos --===============9010457190405382552== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41875/#review113743 ----------------------------------------------------------- Ship it! As a patch it looks good, however I feel the reason we created these types inheriting from the STL containers was to add a functionality to them (like in `hashmap` and `hashset`). But I feel this class doesn't add anything to the default `std::list` and indeed it removes functionality since now there's no access to all the different constructors from `std::list` and we run in the risk of leaking memory (One should not create pointers to any, `List`, `hashmap` or `hashset`). My preferred solution would be to delete this convenience class, and use a typedef while the code that uses it is cleaned up, or just remove it altogether and cleanup in one pass. - Alexander Rojas On Jan. 8, 2016, 12:38 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41875/ > ----------------------------------------------------------- > > (Updated Jan. 8, 2016, 12:38 p.m.) > > > Review request for mesos, Ben Mahler and Till Toenshoff. > > > Bugs: MESOS-4273 > https://issues.apache.org/jira/browse/MESOS-4273 > > > Repository: mesos > > > Description > ------- > > Provided List constructor from std::initializer_list. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/list.hpp 864d8c9498d66f14ab6fc531965c12fb397b5fe5 > 3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 62ad461eb228b688f1ceac16cfb003561ed5a806 > 3rdparty/libprocess/3rdparty/stout/tests/list_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/41875/diff/ > > > Testing > ------- > > make check (OS X 10.10.5 and Debian 8) > > > Thanks, > > Benjamin Bannier > > --===============9010457190405382552==--