You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Alexander Rukletsov <al...@mesosphere.io> on 2015/03/03 21:11:13 UTC
Review Request 31699: Added reserving c-tor to JSON::Array.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-2353
https://issues.apache.org/jira/browse/MESOS-2353
Repository: mesos
Description
-------
See summary.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
Diff: https://reviews.apache.org/r/31699/diff/
Testing
-------
make check (OS X 10.9.5, Ubuntu 14.04)
Thanks,
Alexander Rukletsov
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Michael Park <mc...@gmail.com>.
> On March 4, 2015, 4:13 a.m., Michael Park wrote:
> > I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
> >
> > Currently,
> >
> > ```cpp
> > JSON::Array array(N);
> > // array.values.size() == 0
> > ```
> >
> > The equivalent-looking pattern is deceiving:
> >
> > ```cpp
> > std::vector v(N);
> > // v.size() == N
> > ```
> >
> > I think it would be less confusing for especially for people who are familiar with the standard library to keep:
> >
> > ```cpp
> > JSON::Array array;
> > array.reserve(N);
> > // array.values.size() == N
> > ```
> >
> > ```cpp
> > std::vector v;
> > v.reserve(N);
> > // v.size() == N
> > ```
>
> Michael Park wrote:
> Oops, the second pair of examples should read:
>
> ```cpp
> JSON::Array array;
> array.reserve(N);
> // array.values.size() == 0
> ```
>
> ```cpp
> std::vector v;
> v.reserve(N);
> // v.size() == 0
> ```
>
> Alexander Rukletsov wrote:
> Vector and `JSON::Array` are rather different in a way they are used. There is not much sense to create a `JSON::Array` populated with variants. From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code. Therefore I don't see much value in maintaining the symmetry with `std::vector`.
>
> Michael Park wrote:
> > Vector and `JSON::Array` are rather different in a way they are used. [...] From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code.
>
> Having looked at the usage of `JSON::Array`, it looks to me like we simply use the public member `Array::values`. So we could obviously do `array.values.reserve(N);` the same as we do `array.values.push_back(elem);`. So "reserving capacity seems like a required feature" is covered, which means the main motivation is to "save us lines of code". Am I correct?
>
> > There is not much sense to create a `JSON::Array` populated with variants.
>
> What does this mean?
>
> Alexander Rukletsov wrote:
> > the main motivation is to "save us lines of code"
>
> That's correct.
>
> > What does this mean?
>
> Unary c-tor of `std::vector` fills the collection with default values. I don't see much sense having such a c-tor for `JSON::Array`.
> Unary c-tor of std::vector fills the collection with default values. I don't see much sense having such a c-tor for JSON::Array.
Oh, yes. I wasn't suggesting we add a constructor to `JSON::Array` that default-constructs. It was just that when I saw `JSON::Array array(size);`, I thought "I guess this is similar to `std::vector` does? but if it does it's not doing the right thing..." then had to look at the implementation to realize it does `reserve`. Anyway, perhaps that's just me. Feel free to ignore :)
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 4, 2015, 4:13 a.m., Michael Park wrote:
> > I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
> >
> > Currently,
> >
> > ```cpp
> > JSON::Array array(N);
> > // array.values.size() == 0
> > ```
> >
> > The equivalent-looking pattern is deceiving:
> >
> > ```cpp
> > std::vector v(N);
> > // v.size() == N
> > ```
> >
> > I think it would be less confusing for especially for people who are familiar with the standard library to keep:
> >
> > ```cpp
> > JSON::Array array;
> > array.reserve(N);
> > // array.values.size() == N
> > ```
> >
> > ```cpp
> > std::vector v;
> > v.reserve(N);
> > // v.size() == N
> > ```
>
> Michael Park wrote:
> Oops, the second pair of examples should read:
>
> ```cpp
> JSON::Array array;
> array.reserve(N);
> // array.values.size() == 0
> ```
>
> ```cpp
> std::vector v;
> v.reserve(N);
> // v.size() == 0
> ```
>
> Alexander Rukletsov wrote:
> Vector and `JSON::Array` are rather different in a way they are used. There is not much sense to create a `JSON::Array` populated with variants. From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code. Therefore I don't see much value in maintaining the symmetry with `std::vector`.
>
> Michael Park wrote:
> > Vector and `JSON::Array` are rather different in a way they are used. [...] From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code.
>
> Having looked at the usage of `JSON::Array`, it looks to me like we simply use the public member `Array::values`. So we could obviously do `array.values.reserve(N);` the same as we do `array.values.push_back(elem);`. So "reserving capacity seems like a required feature" is covered, which means the main motivation is to "save us lines of code". Am I correct?
>
> > There is not much sense to create a `JSON::Array` populated with variants.
>
> What does this mean?
> the main motivation is to "save us lines of code"
That's correct.
> What does this mean?
Unary c-tor of `std::vector` fills the collection with default values. I don't see much sense having such a c-tor for `JSON::Array`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Michael Park <mc...@gmail.com>.
> On March 4, 2015, 4:13 a.m., Michael Park wrote:
> > I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
> >
> > Currently,
> >
> > ```cpp
> > JSON::Array array(N);
> > // array.values.size() == 0
> > ```
> >
> > The equivalent-looking pattern is deceiving:
> >
> > ```cpp
> > std::vector v(N);
> > // v.size() == N
> > ```
> >
> > I think it would be less confusing for especially for people who are familiar with the standard library to keep:
> >
> > ```cpp
> > JSON::Array array;
> > array.reserve(N);
> > // array.values.size() == N
> > ```
> >
> > ```cpp
> > std::vector v;
> > v.reserve(N);
> > // v.size() == N
> > ```
>
> Michael Park wrote:
> Oops, the second pair of examples should read:
>
> ```cpp
> JSON::Array array;
> array.reserve(N);
> // array.values.size() == 0
> ```
>
> ```cpp
> std::vector v;
> v.reserve(N);
> // v.size() == 0
> ```
>
> Alexander Rukletsov wrote:
> Vector and `JSON::Array` are rather different in a way they are used. There is not much sense to create a `JSON::Array` populated with variants. From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code. Therefore I don't see much value in maintaining the symmetry with `std::vector`.
> Vector and `JSON::Array` are rather different in a way they are used. [...] From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code.
Having looked at the usage of `JSON::Array`, it looks to me like we simply use the public member `Array::values`. So we could obviously do `array.values.reserve(N);` the same as we do `array.values.push_back(elem);`. So "reserving capacity seems like a required feature" is covered, which means the main motivation is to "save us lines of code". Am I correct?
> There is not much sense to create a `JSON::Array` populated with variants.
What does this mean?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 4, 2015, 4:13 a.m., Michael Park wrote:
> > I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
> >
> > Currently,
> >
> > ```cpp
> > JSON::Array array(N);
> > // array.values.size() == 0
> > ```
> >
> > The equivalent-looking pattern is deceiving:
> >
> > ```cpp
> > std::vector v(N);
> > // v.size() == N
> > ```
> >
> > I think it would be less confusing for especially for people who are familiar with the standard library to keep:
> >
> > ```cpp
> > JSON::Array array;
> > array.reserve(N);
> > // array.values.size() == N
> > ```
> >
> > ```cpp
> > std::vector v;
> > v.reserve(N);
> > // v.size() == N
> > ```
>
> Michael Park wrote:
> Oops, the second pair of examples should read:
>
> ```cpp
> JSON::Array array;
> array.reserve(N);
> // array.values.size() == 0
> ```
>
> ```cpp
> std::vector v;
> v.reserve(N);
> // v.size() == 0
> ```
Vector and `JSON::Array` are rather different in a way they are used. There is not much sense to create a `JSON::Array` populated with variants. From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code. Therefore I don't see much value in maintaining the symmetry with `std::vector`.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Michael Park <mc...@gmail.com>.
> On March 4, 2015, 4:13 a.m., Michael Park wrote:
> > I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
> >
> > Currently,
> >
> > ```cpp
> > JSON::Array array(N);
> > // array.values.size() == 0
> > ```
> >
> > The equivalent-looking pattern is deceiving:
> >
> > ```cpp
> > std::vector v(N);
> > // v.size() == N
> > ```
> >
> > I think it would be less confusing for especially for people who are familiar with the standard library to keep:
> >
> > ```cpp
> > JSON::Array array;
> > array.reserve(N);
> > // array.values.size() == N
> > ```
> >
> > ```cpp
> > std::vector v;
> > v.reserve(N);
> > // v.size() == N
> > ```
Oops, the second pair of examples should read:
```cpp
JSON::Array array;
array.reserve(N);
// array.values.size() == 0
```
```cpp
std::vector v;
v.reserve(N);
// v.size() == 0
```
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Alexander Rukletsov <al...@mesosphere.io>.
> On March 4, 2015, 4:13 a.m., Michael Park wrote:
> > I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
> >
> > Currently,
> >
> > ```cpp
> > JSON::Array array(N);
> > // array.values.size() == 0
> > ```
> >
> > The equivalent-looking pattern is deceiving:
> >
> > ```cpp
> > std::vector v(N);
> > // v.size() == N
> > ```
> >
> > I think it would be less confusing for especially for people who are familiar with the standard library to keep:
> >
> > ```cpp
> > JSON::Array array;
> > array.reserve(N);
> > // array.values.size() == N
> > ```
> >
> > ```cpp
> > std::vector v;
> > v.reserve(N);
> > // v.size() == N
> > ```
>
> Michael Park wrote:
> Oops, the second pair of examples should read:
>
> ```cpp
> JSON::Array array;
> array.reserve(N);
> // array.values.size() == 0
> ```
>
> ```cpp
> std::vector v;
> v.reserve(N);
> // v.size() == 0
> ```
>
> Alexander Rukletsov wrote:
> Vector and `JSON::Array` are rather different in a way they are used. There is not much sense to create a `JSON::Array` populated with variants. From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code. Therefore I don't see much value in maintaining the symmetry with `std::vector`.
>
> Michael Park wrote:
> > Vector and `JSON::Array` are rather different in a way they are used. [...] From the other side, reserving capacity seems like a required feature, and having such a c-tor will save us lines of code.
>
> Having looked at the usage of `JSON::Array`, it looks to me like we simply use the public member `Array::values`. So we could obviously do `array.values.reserve(N);` the same as we do `array.values.push_back(elem);`. So "reserving capacity seems like a required feature" is covered, which means the main motivation is to "save us lines of code". Am I correct?
>
> > There is not much sense to create a `JSON::Array` populated with variants.
>
> What does this mean?
>
> Alexander Rukletsov wrote:
> > the main motivation is to "save us lines of code"
>
> That's correct.
>
> > What does this mean?
>
> Unary c-tor of `std::vector` fills the collection with default values. I don't see much sense having such a c-tor for `JSON::Array`.
>
> Michael Park wrote:
> > Unary c-tor of std::vector fills the collection with default values. I don't see much sense having such a c-tor for JSON::Array.
>
> Oh, yes. I wasn't suggesting we add a constructor to `JSON::Array` that default-constructs. It was just that when I saw `JSON::Array array(size);`, I thought "I guess this is similar to `std::vector` does? but if it does it's not doing the right thing..." then had to look at the implementation to realize it does `reserve`. Anyway, perhaps that's just me. Feel free to ignore :)
You concern is totally valid, however, I think, the benfit outweights the surprise effect.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75147
-----------------------------------------------------------
I think there's value in keeping `JSON::Array` to have the symmetric interface as `std::vector`.
Currently,
```cpp
JSON::Array array(N);
// array.values.size() == 0
```
The equivalent-looking pattern is deceiving:
```cpp
std::vector v(N);
// v.size() == N
```
I think it would be less confusing for especially for people who are familiar with the standard library to keep:
```cpp
JSON::Array array;
array.reserve(N);
// array.values.size() == N
```
```cpp
std::vector v;
v.reserve(N);
// v.size() == N
```
- Michael Park
On March 3, 2015, 8:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 8:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 31699: Added reserving c-tor to JSON::Array.
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31699/#review75028
-----------------------------------------------------------
Ship it!
Ship It!
- Dominic Hamon
On March 3, 2015, 12:11 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31699/
> -----------------------------------------------------------
>
> (Updated March 3, 2015, 12:11 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2353
> https://issues.apache.org/jira/browse/MESOS-2353
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 334c898906018be6e663f53815abbe047806b95c
>
> Diff: https://reviews.apache.org/r/31699/diff/
>
>
> Testing
> -------
>
> make check (OS X 10.9.5, Ubuntu 14.04)
>
>
> Thanks,
>
> Alexander Rukletsov
>
>