> On Sept. 12, 2017, 5:21 p.m., Greg Mann wrote: > > src/tests/mesos.hpp > > Lines 512-522 (original), 620-638 (patched) > > <https://reviews.apache.org/r/62197/diff/3/?file=1819180#file1819180line621> > > > > I'm not sure why this overload was added originally, since `const > > char*` is implicitly convertible to `std::string`. I tried removing this > > overload as well and it compiled OK, so perhaps we can eliminate it?
Good idea, removed it. > On Sept. 12, 2017, 5:21 p.m., Greg Mann wrote: > > src/tests/mesos.hpp > > Lines 641-658 (patched) > > <https://reviews.apache.org/r/62197/diff/3/?file=1819180#file1819180line642> > > > > I suspected that this overload wasn't strictly necessary, so I removed > > it and indeed compilation succeeded. Did you add this proactively to avoid > > the need to add it in the future? > > > > I think we should only add overloads which are actually used in the > > current codebase. Could you verify which overloads in this patch are > > actually used? I added it for consistency with the other method, but we're removing it, so I'm also removing this one. The other overloads are being used, even though we could probably remove a few ones if we do a sweeping change to make all call sites pass the command as a `CommandInfo` instead of as a string. - Gastón ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/62197/#review185216 ----------------------------------------------------------- On Sept. 12, 2017, 7:47 p.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/62197/ > ----------------------------------------------------------- > > (Updated Sept. 12, 2017, 7:47 p.m.) > > > Review request for mesos, Benjamin Bannier, Greg Mann, and Joerg Schad. > > > Bugs: MESOS-7877 > https://issues.apache.org/jira/browse/MESOS-7877 > > > Repository: mesos > > > Description > ------- > > These new overloads make it possible to specify a framework ID, executor > resources as a proto, and the executor ID also as a proto. > > > Diffs > ----- > > src/tests/mesos.hpp 444c75763aea995708db0b17eafee2d22c912554 > > > Diff: https://reviews.apache.org/r/62197/diff/5/ > > > Testing > ------- > > `make check` still passes on GNU/Linux. > > > Thanks, > > Gastón Kleiman > >
