----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53387/#review154626 -----------------------------------------------------------
Fix it, then Ship it! Thanks for fixing these Neil! I opened some issues related to MESOS-5122 (one of my semi-secret pet peeves); feel free to drop deliberately. src/tests/containerizer/mesos_containerizer_tests.cpp (line 906) <https://reviews.apache.org/r/53387/#comment224257> Could you write this as Owned<Launcher> launcher(new TestLauncher(Owned<Launcher>(launcher_.get()))); to document that semantically an `Owned<T>` can only be constructed from an rvalue? Alternatively you could `std::move` the RHS, but the current nesting of types is already hard to follow. src/tests/containerizer/mesos_containerizer_tests.cpp (line 933) <https://reviews.apache.org/r/53387/#comment224254> Since an `Owned<T>` has unique ownership it semantically should not be copied (see e.g., MESOS-5122 for the related cleanup ticket). Would you mind `std::move`'ing `launcher`? src/tests/containerizer/mesos_containerizer_tests.cpp (line 1002) <https://reviews.apache.org/r/53387/#comment224258> Could you write this as Owned<Launcher> launcher(new TestLauncher(Owned<Launcher>(launcher_.get()))); to document that semantically an `Owned<T>` can only be constructed from an rvalue? Alternatively you could `std::move` the RHS, but the current nesting of types is already hard to follow. src/tests/containerizer/mesos_containerizer_tests.cpp (line 1029) <https://reviews.apache.org/r/53387/#comment224262> We should `std::move(launcher)` here to be semantically correct. src/tests/containerizer/mesos_containerizer_tests.cpp (line 1100) <https://reviews.apache.org/r/53387/#comment224259> Could you write this as Owned<Launcher> launcher(new TestLauncher(Owned<Launcher>(launcher_.get()))); to document that semantically an `Owned<T>` can only be constructed from an rvalue? Alternatively you could `std::move` the `TestLauncher`, but the current nesting of types is already hard to follow. src/tests/containerizer/mesos_containerizer_tests.cpp (line 1215) <https://reviews.apache.org/r/53387/#comment224261> This can be written as Owned<Launcher> launcher(testLauncher); and also documents that semantically an `Owned<T>` can only be constructed from an rvalue. Alternatively you could `std::move` the RHS. src/tests/containerizer/mesos_containerizer_tests.cpp (line 1232) <https://reviews.apache.org/r/53387/#comment224256> I believe we should `std::move` here as well. - Benjamin Bannier On Nov. 2, 2016, 5:24 p.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53387/ > ----------------------------------------------------------- > > (Updated Nov. 2, 2016, 5:24 p.m.) > > > Review request for mesos and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > `clang-tidy` points out, rightly, that an `ASSERT` failure can result in > leaking some heap-allocated values that aren't wrapped in a smart > pointer. This commit fixes the cases that `clang-tidy` complains about > by wrapping the values in `Owned<T>`. > > Note that there are many other places in the tests that leak resources > if an exception occurs. The proper fix is usually to use a smart pointer > rather than a raw pointer. However, this is not always easy/clean, in > part because the current `Owned<T>` and `Shared<T>` types do not support > inheritance (MESOS-6496). So for now, just fix the cases that clang-tidy > complains about. > > > Diffs > ----- > > src/tests/containerizer/mesos_containerizer_tests.cpp > 4df537747d84daa68c29e2d05b22fa386a4a16db > > Diff: https://reviews.apache.org/r/53387/diff/ > > > Testing > ------- > > `make check` > > Verified that observed `clang-tidy` warnings go away with this change. > > > Thanks, > > Neil Conway > >
