> On June 12, 2015, 11:15 a.m., Adam B wrote: > > Looks like Cody already made path::join() variadic in commit > > b08fccf8f5ea325b8c38055b5f2c03509744dd9b "Switched path::join() to be > > variadic". How is your patch an improvement? What problem is it solving? > > > > Please remove the "Might have some C++ style issues..." line in your > > description, since the apply-review script uses the description to create > > the commit message. You can add any comments to reviewers in the Testing > > section, since that is ignored by apply-review. > > > > Your Testing Done section says that you "added some additional tests", but > > I don't see them in this patch. Looks like they were in a previous > > revision, but removed. Please add them back or update the Testing section. > > > > ReviewBot says the patch doesn't apply cleanly, so please rebase.
- Cody's patch "b08fccf8f5ea325b8c38055b5f2c03509744dd9b" tried to also optimize how we "append" the path-separator "/" by making trimming calls to remove/add them recusively during the join(...) function calls. This led to some errors in the implementation and made it deviate from the earlier join(...) behavior i.e. prior to it being variadic. This patch takes a much simpler route at tackling the issue by just making the function variadic minus the trimming optimizations ( they don't even buy us much anyways in terms of optimization ). Hence, this patch essentially tries to make the join(...) variadic by trying to keep the behavior as close to the previous one as possible. - Removed the "might have some C++ style issues..." line from description. - Added the deleted "additional tests". My bad, I was essentially submitting diffs based on Cody's comments without rebasing but just the "new" diff that addressed the comments. Putting them back now. - Rebased from master, so ReviewBot hopefully should not complain anymore. - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87699 ----------------------------------------------------------- On June 12, 2015, 5:17 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35179/ > ----------------------------------------------------------- > > (Updated June 12, 2015, 5:17 p.m.) > > > Review request for mesos, Adam B and Cody Maloney. > > > Bugs: MESOS-1733 > https://issues.apache.org/jira/browse/MESOS-1733 > > > Repository: mesos > > > Description > ------- > > This change takes an un-complicated/naive route ( no trimming of values etc ) > at making path::join(...) variadic mainly in order to preserve the earlier > over-loaded join functionality. > > > Diffs > ----- > > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp > d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c > 3rdparty/libprocess/3rdparty/stout/include/stout/require.hpp PRE-CREATION > 3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp > cf354125687e0f60b6d5b105f19d75e4436f21bf > > Diff: https://reviews.apache.org/r/35179/diff/ > > > Testing > ------- > > make check + added some additional tests. > > > Thanks, > > Anand Mazumdar > >
