> On Sept. 30, 2016, 3:36 p.m., Kevin Klues wrote: > > src/slave/containerizer/mesos/paths.hpp, lines 57-60 > > <https://reviews.apache.org/r/52415/diff/1/?file=1516702#file1516702line57> > > > > To keep similar semantics as before, maybe it makes sense to set > > defaults for separator and mode: > > > > ``` > > std::string buildPath( > > const ContainerID& containerId, > > const std::string& separator = "", > > const Mode& mode = Mode::JOIN); > > ```
In fact, I like it being more explicit :) > On Sept. 30, 2016, 3:36 p.m., Kevin Klues wrote: > > src/slave/containerizer/mesos/paths.hpp, line 41 > > <https://reviews.apache.org/r/52415/diff/1/?file=1516702#file1516702line41> > > > > Should we use an `enum class` here? Without it, the enum will not be > > typed properly under `Mode`, so you could write things like: > > > > `containerizer::paths::JOIN` > > > > (which you currently do, and is totally legal) > > > > With `enum class` you are forced to write: > > > > `containerizer::paths::Mode::JOIN` > > > > Not sure which is better, but it would be good to be explicit about it. This is consistent with the Mode we have in 'strings'. But I agree with you probably having Mode::JOIN is better. I'll follow up with a patch. - Jie ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52415/#review151025 ----------------------------------------------------------- On Sept. 30, 2016, 5:14 a.m., Jie Yu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52415/ > ----------------------------------------------------------- > > (Updated Sept. 30, 2016, 5:14 a.m.) > > > Review request for mesos, Benjamin Hindman and Kevin Klues. > > > Repository: mesos > > > Description > ------- > > Extended buildPath to support more modes. > > > Diffs > ----- > > src/Makefile.am f093000e0282a8d5ac17e7ba33711690ccdfe68a > src/slave/containerizer/mesos/linux_launcher.cpp > 1bce077e4aa97425b9cbdf8576a5dd52851c044e > src/slave/containerizer/mesos/paths.hpp > 1051c219c55253d03199045b6d2f43377ae93e53 > src/slave/containerizer/mesos/paths.cpp > 6c6b4dcc39fbc00485552caab88457918e622e08 > src/tests/containerizer/mesos_containerizer_paths_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/52415/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Jie Yu > >
