Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review88312 --- Ship it! Looks great! I'll get this committed tonight - Adam B On June 15, 2015, 4:23 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 4:23 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review88059 --- Ship it! Ship It! - Michael Park On June 15, 2015, 11:23 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 11:23 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 3:25 p.m.) Review request for mesos, Adam B and Cody Maloney. Changes --- Address Adam's review comments. 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 (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87933 --- Patch looks great! Reviews applied: [35179] All tests passed. - Mesos ReviewBot On June 15, 2015, 3:25 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 3:25 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87999 --- Patch looks great! Reviews applied: [35179] All tests passed. - Mesos ReviewBot On June 15, 2015, 11:23 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 11:23 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87989 --- 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140398 Maybe `s/Default/Base/`? 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140400 Not yours, but could you fix the formatting here to not wrap after `return` please? ``` return strings::remove(path1, /, strings::SUFFIX) + / + strings::remove(path2, /, strings::PREFIX); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140401 Not yours, but could you fix the formatting here to not wrap after `return` please? ``` return strings::remove(path1, /, strings::SUFFIX) + / + strings::remove(path2, /, strings::PREFIX); ``` 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp https://reviews.apache.org/r/35179/#comment140399 `s/path1 ,/path1,` - Michael Park On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 10:45 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
On June 15, 2015, 10:57 p.m., Michael Park wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 49-51 https://reviews.apache.org/r/35179/diff/6/?file=984730#file984730line49 Not yours, but could you fix the formatting here to not wrap after `return` please? ``` return strings::remove(path1, /, strings::SUFFIX) + / + strings::remove(path2, /, strings::PREFIX); ``` Not sure why this got duplicated. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87989 --- On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 15, 2015, 10:45 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/tests/path_tests.cpp cf354125687e0f60b6d5b105f19d75e4436f21bf Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87699 --- 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. - Adam B On June 12, 2015, 3:32 a.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 12, 2015, 3:32 a.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. Might have some C++ style issues owing to this being my first commit here. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 12, 2015, 7:04 p.m.) Review request for mesos, Adam B and Cody Maloney. Changes --- Amended commit to include email info to make review-bot happy. 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 (updated) - 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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- 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 (updated) --- 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 (updated) - 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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
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
Re: Review Request 35179: MESOS-1733 Variadic Path Join
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 9, 2015, 3:56 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. Might have some C++ style issues owing to this being my first commit here. Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
On June 9, 2015, 6:34 a.m., Cody Maloney wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 58 https://reviews.apache.org/r/35179/diff/2/?file=980529#file980529line58 I'm still against using enable_if here, it's a lot of extra complexity (And will be slightly greater compile time, as well as more complexity the compiler has to sort out for runtime). I don't see it providing great value over not using it. Hiding it behind a macro doesn't make it simpler. I removed the usage but is it fine with you if I keep require.hpp i.e. the macro intact ? It might benefit others who find the std::enable_if syntax too cumber-some ? ( I can see a couple of places in the code-base already where this can be used already ) Also, I did not quite understand your argument around a lot of extra complexity. Did you mean syntaxtic complexity around std::enable_if ? ( That is what the macro was trying to achieve to make it as painless to use it. ) As for the compile-time complexity , I wouldn't think about it too much with the modern compilers et al. - Anand --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87140 --- On June 9, 2015, 3:56 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 9, 2015, 3:56 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. Might have some C++ style issues owing to this being my first commit here. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
Re: Review Request 35179: MESOS-1733 Variadic Path Join
On June 9, 2015, 6:34 a.m., Cody Maloney wrote: 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 58 https://reviews.apache.org/r/35179/diff/2/?file=980529#file980529line58 I'm still against using enable_if here, it's a lot of extra complexity (And will be slightly greater compile time, as well as more complexity the compiler has to sort out for runtime). I don't see it providing great value over not using it. Hiding it behind a macro doesn't make it simpler. Anand Mazumdar wrote: I removed the usage but is it fine with you if I keep require.hpp i.e. the macro intact ? It might benefit others who find the std::enable_if syntax too cumber-some ? ( I can see a couple of places in the code-base already where this can be used already ) Also, I did not quite understand your argument around a lot of extra complexity. Did you mean syntaxtic complexity around std::enable_if ? ( That is what the macro was trying to achieve to make it as painless to use it. ) As for the compile-time complexity , I wouldn't think about it too much with the modern compilers et al. Just found a good presentation about the concepts in general: https://youtu.be/eR34r7HOU14?t=27m13s. At the very end of that presentation (Around 1 hour, 22 minutes in), Chandler talks in specific about doing stuff inside recursive variadic templates. More or less, the template ends up generating a mountain of code and function calls which is very hard to analyze as a compiler back end. - Cody --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review87140 --- On June 9, 2015, 3:56 p.m., Anand Mazumdar wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/ --- (Updated June 9, 2015, 3:56 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. Might have some C++ style issues owing to this being my first commit here. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c Diff: https://reviews.apache.org/r/35179/diff/ Testing --- make check + added some additional tests. Thanks, Anand Mazumdar
