Re: Review Request 49424: Added an abstraction os::raw::Argv in stout.

[Jie Yu]

> On July 2, 2016, 5:11 p.m., Joris Van Remoortere wrote:
> > 3rdparty/stout/include/stout/os/raw/argv.hpp, line 40
> > 
> >
> > It seems like this extra vector and the subsequent copy into the argv 
> > array is only necessary because you're supporting structures with an 
> > unknown size.
> > 
> > Would this be simpler if you restricted to inputs that support 
> > `.size()`?

Yes, it would be simpler, but I still have to deal with issues like size() of 
protobuf repeated field returns an int while other std containers return size_t 
(unsigned). This is just pointer copies so it won't be too bad.


- Jie


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


On June 30, 2016, 4:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49424/
> ---
> 
> (Updated June 30, 2016, 4:18 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an abstraction os::raw::Argv in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
>   3rdparty/stout/include/stout/os.hpp 
> 53b00932693fba7cf6514da6a519269a904de345 
>   3rdparty/stout/include/stout/os/raw/argv.hpp PRE-CREATION 
>   3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 
> 
> Diff: https://reviews.apache.org/r/49424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49424: Added an abstraction os::raw::Argv in stout.

[Joris Van Remoortere]

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




3rdparty/stout/include/stout/os/raw/argv.hpp (line 27)


I know you want to say `NULL` to be explicit here. Consider using `nullptr` 
to avoid tripping people grepping for `NULL`.



3rdparty/stout/include/stout/os/raw/argv.hpp (line 40)


It seems like this extra vector and the subsequent copy into the argv array 
is only necessary because you're supporting structures with an unknown size.

Would this be simpler if you restricted to inputs that support `.size()`?



3rdparty/stout/include/stout/os/raw/argv.hpp (line 43)


Wouldn't hurt to have a small comment here mentioning that `strcpy` does 
the null termination.


- Joris Van Remoortere


On June 30, 2016, 4:18 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49424/
> ---
> 
> (Updated June 30, 2016, 4:18 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an abstraction os::raw::Argv in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
>   3rdparty/stout/include/stout/os.hpp 
> 53b00932693fba7cf6514da6a519269a904de345 
>   3rdparty/stout/include/stout/os/raw/argv.hpp PRE-CREATION 
>   3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 
> 
> Diff: https://reviews.apache.org/r/49424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49424: Added an abstraction os::raw::Argv in stout.

[Gilbert Song]

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


Ship it!




LGTM!

- Gilbert Song


On June 30, 2016, 9:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49424/
> ---
> 
> (Updated June 30, 2016, 9:18 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an abstraction os::raw::Argv in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
>   3rdparty/stout/include/stout/os.hpp 
> 53b00932693fba7cf6514da6a519269a904de345 
>   3rdparty/stout/include/stout/os/raw/argv.hpp PRE-CREATION 
>   3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 
> 
> Diff: https://reviews.apache.org/r/49424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49424: Added an abstraction os::raw::Argv in stout.

[Jie Yu]

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

(Updated June 30, 2016, 4:18 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Added the file to makefile.


Repository: mesos


Description
---

Added an abstraction os::raw::Argv in stout.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am f10c836c1ac008cc4055741648b5e7dd697e4c1e 
  3rdparty/stout/include/stout/os.hpp 53b00932693fba7cf6514da6a519269a904de345 
  3rdparty/stout/include/stout/os/raw/argv.hpp PRE-CREATION 
  3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 

Diff: https://reviews.apache.org/r/49424/diff/


Testing
---

make check


Thanks,

Jie Yu

Review Request 49424: Added an abstraction os::raw::Argv in stout.

[Jie Yu]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

Added an abstraction os::raw::Argv in stout.


Diffs
-

  3rdparty/stout/include/stout/os.hpp 53b00932693fba7cf6514da6a519269a904de345 
  3rdparty/stout/include/stout/os/raw/argv.hpp PRE-CREATION 
  3rdparty/stout/tests/os_tests.cpp 786da14addf62be5f5270f156fb1e011d3f403e3 

Diff: https://reviews.apache.org/r/49424/diff/


Testing
---

make check


Thanks,

Jie Yu