Re: Review Request 35743: flags: fixed const'ness of load

[Ben Mahler]

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



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (lines 341 - 345)


Let's try to avoid proliferating 'const' qualifiers on all of our locally 
scoped variables, as to do it consistently would be very verbose. For example, 
you didn't do it to the const 'load' result from flags.load below.

Perhaps for this particular case, since it resembles a literal constant, we 
could keep 'const' and call it VALUES. Some food for thought.


- Ben Mahler


On June 25, 2015, 12:29 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 25, 2015, 12:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/utils.hpp 
> 09a1dcd3b3a082544d221fbfeab9a3d3d9f85e2f 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35743]

All tests passed.

- Mesos ReviewBot


On June 25, 2015, 12:29 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 25, 2015, 12:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/include/stout/utils.hpp 
> 09a1dcd3b3a082544d221fbfeab9a3d3d9f85e2f 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Jojy Varghese]

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

(Updated June 25, 2015, 12:29 a.m.)


Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
Toenshoff.


Changes
---

addressed review comments. 
Note: Chose to move the util function to util file as opposed to calling 
std::extent as std::extent looked like a heavy hammer to solve something simple.


Repository: mesos


Description
---

Changes:
  - replacing argument arrays with static initializers in tests.
  - making the argument 'argv' const in load method to reflect the semantics.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
7584cb871d02ad01021f0c3439ea205736d4f6b4 
  3rdparty/libprocess/3rdparty/stout/include/stout/utils.hpp 
09a1dcd3b3a082544d221fbfeab9a3d3d9f85e2f 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
c2c6a6ac97044f2317418295f48d75e94de4112b 

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


Testing
---

make check


Thanks,

Jojy Varghese

Re: Review Request 35743: flags: fixed const'ness of load

[Till Toenshoff]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (lines 33 - 39)


Should live somewhere else? ... how about stout/utils.hpp?


- Till Toenshoff


On June 23, 2015, 9:51 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 23, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Anand Mazumdar]

> On June 22, 2015, 10:02 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 418
> > 
> >
> > boost::size(...) would suffice here too albeit minus the small trivial 
> > run-time cost.
> 
> Jojy Varghese wrote:
> Not sure if adding a boost dependency just for size operator would get us 
> anything here.
> 
> Anand Mazumdar wrote:
> Then can we change these to std::array and just invoke the size() 
> function  ?
> 
> (Posting it again here , my bad , instead of adding a new comment)
> 
> Jojy Varghese wrote:
> thats a possibility but at teh cost/inconvenience of having to change the 
> std::array size argument every time we add/remove an argument in the test.

How about then using std::extent here (something like 
std::extent::value) ?

PS: All I want for us is to not re-invent the wheel and add another helper 
function in our code-base. :-)


- Anand


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


On June 23, 2015, 9:51 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 23, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Jojy Varghese]

> On June 22, 2015, 10:02 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 418
> > 
> >
> > boost::size(...) would suffice here too albeit minus the small trivial 
> > run-time cost.
> 
> Jojy Varghese wrote:
> Not sure if adding a boost dependency just for size operator would get us 
> anything here.
> 
> Anand Mazumdar wrote:
> Then can we change these to std::array and just invoke the size() 
> function  ?
> 
> (Posting it again here , my bad , instead of adding a new comment)

thats a possibility but at teh cost/inconvenience of having to change the 
std::array size argument every time we add/remove an argument in the test.


- Jojy


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


On June 23, 2015, 9:51 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 23, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Anand Mazumdar]

> On June 22, 2015, 10:02 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 418
> > 
> >
> > boost::size(...) would suffice here too albeit minus the small trivial 
> > run-time cost.
> 
> Jojy Varghese wrote:
> Not sure if adding a boost dependency just for size operator would get us 
> anything here.

Then can we change these to std::array and just invoke the size() function  ?

(Posting it again here , my bad , instead of adding a new comment)


- Anand


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


On June 23, 2015, 9:51 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 23, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Anand Mazumdar]

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



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (line 413)


Then can we change these to std::array and just invoke the size() function  
?


- Anand Mazumdar


On June 23, 2015, 9:51 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 23, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Joris Van Remoortere, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Jojy Varghese]

> On June 22, 2015, 10:02 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp, line 418
> > 
> >
> > boost::size(...) would suffice here too albeit minus the small trivial 
> > run-time cost.

Not sure if adding a boost dependency just for size operator would get us 
anything here.


- Jojy


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


On June 22, 2015, 9:27 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 22, 2015, 9:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35743]

All tests passed.

- Mesos ReviewBot


On June 22, 2015, 9:27 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 22, 2015, 9:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 35743: flags: fixed const'ness of load

[Anand Mazumdar]

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

Ship it!



3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp (line 413)


boost::size(...) would suffice here too albeit minus the small trivial 
run-time cost.


- Anand Mazumdar


On June 22, 2015, 9:27 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35743/
> ---
> 
> (Updated June 22, 2015, 9:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - replacing argument arrays with static initializers in tests.
>   - making the argument 'argv' const in load method to reflect the semantics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> 7584cb871d02ad01021f0c3439ea205736d4f6b4 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> c2c6a6ac97044f2317418295f48d75e94de4112b 
> 
> Diff: https://reviews.apache.org/r/35743/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>