Re: Review Request 71878: Added iteration support to stout's Path.

2019-12-09 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71878/
---

(Updated Dec. 9, 2019, 5:48 p.m.)


Review request for mesos and Benno Evers.


Changes
---

Correct changeset


Bugs: MESOS-10062
https://issues.apache.org/jira/browse/MESOS-10062


Repository: mesos


Description
---

Added iteration support to stout's Path.


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
  3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 


Diff: https://reviews.apache.org/r/71878/diff/3/

Changes: https://reviews.apache.org/r/71878/diff/2-3/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71878: Added iteration support to stout's Path.

2019-12-09 Thread Benjamin Bannier 


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 388 (patched)
> > 
> >
> > This feels a bit more "common" to me:
> > 
> > struct PathComponentIterator {
> > // ..
> > };
> > 
> > typedef const PathComponentIterator const_iterator;
> > 
> > What do you think?

I'd vote to not do this as we do not need it.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 389 (patched)
> > 
> >
> > Using `std::iterator` is going to be deprecated in C++17, apparently 
> > the preferred alternative is to just create typedefs for `value_type`, etc. 
> > in the iterator class directly.

I'll do that since it also documents more what is happening in a more obvious 
way.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 437 (patched)
> > 
> >
> > I think this needs to be a loop, consider e.g. `/home/foobar`

That would be path normalization which I didn't want to do in this class as it 
has a number of other complications. For a path like `/home/foo//bar` we would 
like to generate elements `{"home", "foo", "", "bar"}`. I'll clean up remnants 
of my attempts in this class here and in tests.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 454 (patched)
> > 
> >
> > Should we also check `path == other.path`? Or maybe even `CHECK(path == 
> > other.path)`?

I'll end the hard `CHECK` once I have gotten rid of the end sentinel.


> On Dec. 9, 2019, 3:34 p.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 469 (patched)
> > 
> >
> > I don't completely understand why we need a sentinel, isn't 
> > `path->end()` suitable for everything we need to do?

I removed as per our offline discussion.


- Benjamin


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71878/#review218978
---


On Dec. 9, 2019, 5:38 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71878/
> ---
> 
> (Updated Dec. 9, 2019, 5:38 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added iteration support to stout's Path.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71878/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 71878: Added iteration support to stout's Path.

2019-12-09 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71878/
---

(Updated Dec. 9, 2019, 5:38 p.m.)


Review request for mesos and Benno Evers.


Bugs: MESOS-10062
https://issues.apache.org/jira/browse/MESOS-10062


Repository: mesos


Description
---

Added iteration support to stout's Path.


Diffs (updated)
-

  3rdparty/stout/include/stout/path.hpp 
ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
  3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 


Diff: https://reviews.apache.org/r/71878/diff/2/

Changes: https://reviews.apache.org/r/71878/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 71878: Added iteration support to stout's Path.

2019-12-09 Thread Benno Evers 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71878/#review218978
---




3rdparty/stout/include/stout/path.hpp
Lines 388 (patched)


This feels a bit more "common" to me:

struct PathComponentIterator {
// ..
};

typedef const PathComponentIterator const_iterator;

What do you think?



3rdparty/stout/include/stout/path.hpp
Lines 389 (patched)


Using `std::iterator` is going to be deprecated in C++17, apparently the 
preferred alternative is to just create typedefs for `value_type`, etc. in the 
iterator class directly.



3rdparty/stout/include/stout/path.hpp
Lines 391 (patched)


Shouldn't `value_type` be `std::string`? (or the `string_view`-like class 
from the TODO below)



3rdparty/stout/include/stout/path.hpp
Lines 437 (patched)


I think this needs to be a loop, consider e.g. `/home/foobar`



3rdparty/stout/include/stout/path.hpp
Lines 454 (patched)


Should we also check `path == other.path`? Or maybe even `CHECK(path == 
other.path)`?



3rdparty/stout/include/stout/path.hpp
Lines 469 (patched)


I don't completely understand why we need a sentinel, isn't `path->end()` 
suitable for everything we need to do?



3rdparty/stout/include/stout/path.hpp
Lines 479 (patched)


Why not store a `Path*` here? Then we would not need to have an extra copy 
of `separator` in the iterator.


- Benno Evers


On Dec. 5, 2019, 12:48 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71878/
> ---
> 
> (Updated Dec. 5, 2019, 12:48 p.m.)
> 
> 
> Review request for mesos and Benno Evers.
> 
> 
> Bugs: MESOS-10062
> https://issues.apache.org/jira/browse/MESOS-10062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added iteration support to stout's Path.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/path.hpp 
> ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
>   3rdparty/stout/tests/path_tests.cpp 
> 19dd910a534040468aeb48f15ebdf56dff32bc15 
> 
> 
> Diff: https://reviews.apache.org/r/71878/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 71878: Added iteration support to stout's Path.

2019-12-05 Thread Benjamin Bannier 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71878/
---

Review request for mesos and Benno Evers.


Bugs: MESOS-10062
https://issues.apache.org/jira/browse/MESOS-10062


Repository: mesos


Description
---

Added iteration support to stout's Path.


Diffs
-

  3rdparty/stout/include/stout/path.hpp 
ba1f665ce94b9636d88a7ecce8643c56758f7b5c 
  3rdparty/stout/tests/path_tests.cpp 19dd910a534040468aeb48f15ebdf56dff32bc15 


Diff: https://reviews.apache.org/r/71878/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier