mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Anand Mazumdar" <an...@mesosphere.io>
Subject Re: Review Request 35179: MESOS-1733 Variadic Path Join
Date Fri, 12 Jun 2015 17:18:10 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message