You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jan Schlicht <ja...@mesosphere.io> on 2015/08/07 11:45:56 UTC
Re: Review Request 37188: Added std::hash template specializations.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 11:45 a.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Summary (updated)
-----------------
Added std::hash template specializations.
Repository: mesos
Description (updated)
-------
Added std::hash template specializations.
Diffs
-----
3rdparty/libprocess/include/process/address.hpp be216db823160f5db1dfb4502bf832246fb3df6d
3rdparty/libprocess/include/process/pid.hpp 3bce0bc99e0ebe3ac06ba53155d558fb041cd76c
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review94861
-----------------------------------------------------------
Looks good overall! One question: why did you decide to leave the `hash_value` functions and call it from `std::hash` specializations rather than moving the logic?
- Michael Park
On Aug. 10, 2015, 12:25 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 12:25 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96354
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Aug. 25, 2015, 1:15 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 1:15 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96738
-----------------------------------------------------------
Ship it!
Ship It!
- Michael Park
On Aug. 27, 2015, 4:23 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 4:23 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [2/3] libprocess: Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 6:23 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Move header.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description (updated)
-------
[2/3] libprocess: Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 5:14 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Add result_type, argument_type for std::hash specializations.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96672
-----------------------------------------------------------
There's an instance of `hash_value` still in `3rdparty/libprocess/3rdparty/stout/include/stout/ip.hpp` currently.
Turns out `boost::hash_combine(seed, value)` combines the result of `hash_value(value)` into `seed` which makes this work.
We'll replace `boost::hash_combine` in a follow-up patch, for now let's change `hash_value` for `net::IP` into `std::hash`.
This subsequently requires updating 2 callsites of `boost::hash_combine`, one in `3rdparty/libprocess/include/process/address.hpp` and the other in `3rdparty/libprocess/src/pid.cpp`.
3rdparty/libprocess/src/pid.cpp (line 153)
<https://reviews.apache.org/r/37188/#comment152297>
This is 81 chars, we could either `s/upid/pid/`, or wrap the argument. I'll leave this up to you.
- Michael Park
On Aug. 27, 2015, 11:39 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 11:39 a.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 27, 2015, 1:39 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Fixed open issues.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Aug. 26, 2015, 11:13 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/pid.hpp, line 153
> > <https://reviews.apache.org/r/37188/diff/7/?file=1051129#file1051129line153>
> >
> > there is an extra space hiding here :-)
There was another one in line 140, removed that one as well.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96593
-----------------------------------------------------------
On Aug. 27, 2015, 1:39 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 1:39 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Aug. 26, 2015, 11:13 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, line 806
> > <https://reviews.apache.org/r/37188/diff/7/?file=1051131#file1051131line806>
> >
> > I think we tend to put a new line after an expression that doesn't fit on 1 line.
> > Same for the one below.
I added some new lines.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96593
-----------------------------------------------------------
On Aug. 27, 2015, 1:39 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 1:39 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 26, 2015, 9:13 p.m., Joris Van Remoortere wrote:
> > @Mpark: For the standard hashers, do we want to follow this pattern?
> > ```
> > template <>
> > struct hash<T> {
> >
> > typedef size_t result_type;
> >
> > typedef T argument_type;
> >
> > result_type operator()(const argument_type &that) const {
> > // body
> > }
> >
> > };
> > ```
> >
> > Are we replacing all uses of the hash based boost containers? If so, should we clean up the remaining instances of `hash_value` implementations? Grepping through I still see some.
@Joris: Thanks for the review!
@Jan:
(1) Let's follow the pattern Joris mentions above since the standard technically requires the `result_type`, and `argument_type` typedefs.
(2) I've pointed out the parts where `hash_value` is still left over in the codebase more specifically in the reviews, let's take care of those as well.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96593
-----------------------------------------------------------
On Aug. 27, 2015, 11:39 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 27, 2015, 11:39 a.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96593
-----------------------------------------------------------
@Mpark: For the standard hashers, do we want to follow this pattern?
```
template <>
struct hash<T> {
typedef size_t result_type;
typedef T argument_type;
result_type operator()(const argument_type &that) const {
// body
}
};
```
Are we replacing all uses of the hash based boost containers? If so, should we clean up the remaining instances of `hash_value` implementations? Grepping through I still see some.
3rdparty/libprocess/include/process/pid.hpp (line 153)
<https://reviews.apache.org/r/37188/#comment152176>
there is an extra space hiding here :-)
3rdparty/libprocess/src/tests/http_tests.cpp (line 806)
<https://reviews.apache.org/r/37188/#comment152180>
I think we tend to put a new line after an expression that doesn't fit on 1 line.
Same for the one below.
- Joris Van Remoortere
On Aug. 25, 2015, 1:15 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 1:15 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 25, 2015, 3:15 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Rebase master & fix issue.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Aug. 25, 2015, 2:36 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 677-695
> > <https://reviews.apache.org/r/37188/diff/6/?file=1037577#file1037577line677>
> >
> > I think this might be the right solution for the scope of the changes we're trying to make, but I don't think we'll want to keep this kind of tests.
> >
> > My preference would be to change `URL` to operate on `map` rather than `hashmap`.
>
> Jan Schlicht wrote:
> Agreed!
>
> Michael Park wrote:
> Could you file a JIRA ticket describing the issue and a few simple solutions for it?
> 1. Make `URL` operate on `map` rather than `hashmap`
> 2. Only update the `stringify` function to construct a temporary `map` from the `hashmap` in order to print them out in a deterministic order)
> 3. ... anything else you can think of
>
> Thanks!
https://issues.apache.org/jira/browse/MESOS-3317
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96329
-----------------------------------------------------------
On Aug. 25, 2015, 3:15 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 3:15 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
> On Aug. 25, 2015, 12:36 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 677-695
> > <https://reviews.apache.org/r/37188/diff/6/?file=1037577#file1037577line677>
> >
> > I think this might be the right solution for the scope of the changes we're trying to make, but I don't think we'll want to keep this kind of tests.
> >
> > My preference would be to change `URL` to operate on `map` rather than `hashmap`.
>
> Jan Schlicht wrote:
> Agreed!
Could you file a JIRA ticket describing the issue and a few simple solutions for it?
1. Make `URL` operate on `map` rather than `hashmap`
2. Only update the `stringify` function to construct a temporary `map` from the `hashmap` in order to print them out in a deterministic order)
3. ... anything else you can think of
Thanks!
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96329
-----------------------------------------------------------
On Aug. 25, 2015, 1:15 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 1:15 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Aug. 25, 2015, 2:36 p.m., Michael Park wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, lines 677-695
> > <https://reviews.apache.org/r/37188/diff/6/?file=1037577#file1037577line677>
> >
> > I think this might be the right solution for the scope of the changes we're trying to make, but I don't think we'll want to keep this kind of tests.
> >
> > My preference would be to change `URL` to operate on `map` rather than `hashmap`.
Agreed!
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96329
-----------------------------------------------------------
On Aug. 25, 2015, 3:15 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2015, 3:15 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/pid.cpp f5528aea04a17ce868a0f67b94defdefea18e234
> 3rdparty/libprocess/src/tests/http_tests.cpp c22f9d35b5dc959ce9816d3358ebff92b853bf52
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review96329
-----------------------------------------------------------
3rdparty/libprocess/include/process/pid.hpp (lines 154 - 155)
<https://reviews.apache.org/r/37188/#comment151643>
Like elsewhere, can we move the body of this `hash_value` function into `hash<process::UPID>::operator()` and remove it?
3rdparty/libprocess/src/tests/http_tests.cpp (lines 677 - 695)
<https://reviews.apache.org/r/37188/#comment151645>
I think this might be the right solution for the scope of the changes we're trying to make, but I don't think we'll want to keep this kind of tests.
My preference would be to change `URL` to operate on `map` rather than `hashmap`.
- Michael Park
On Aug. 11, 2015, 9:57 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 11, 2015, 9:57 a.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 11, 2015, 11:57 a.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Remove hash_value functions.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Michael Park <mc...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review94858
-----------------------------------------------------------
3rdparty/libprocess/include/process/address.hpp (line 160)
<https://reviews.apache.org/r/37188/#comment149513>
`s/template<>/template <>/`
- Michael Park
On Aug. 10, 2015, 12:25 p.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 10, 2015, 12:25 p.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
> 3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 2:25 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 10, 2015, 10:52 a.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Rebase master.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp df78f8e525c40b87e734e16979d3315f89e12594
3rdparty/libprocess/include/process/pid.hpp 8d3735c7d5b8f74a7a0ebb8cafe7c7ebee68f5f0
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 1:53 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Changes
-------
Add comment describing hashmap behaviour.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs (updated)
-----
3rdparty/libprocess/include/process/address.hpp be216db823160f5db1dfb4502bf832246fb3df6d
3rdparty/libprocess/include/process/pid.hpp 3bce0bc99e0ebe3ac06ba53155d558fb041cd76c
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/
-----------------------------------------------------------
(Updated Aug. 7, 2015, 12:28 p.m.)
Review request for mesos, Alexander Rojas and Michael Park.
Bugs: MESOS-3217
https://issues.apache.org/jira/browse/MESOS-3217
Repository: mesos
Description
-------
Added std::hash template specializations.
Diffs
-----
3rdparty/libprocess/include/process/address.hpp be216db823160f5db1dfb4502bf832246fb3df6d
3rdparty/libprocess/include/process/pid.hpp 3bce0bc99e0ebe3ac06ba53155d558fb041cd76c
3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
Diff: https://reviews.apache.org/r/37188/diff/
Testing
-------
make check
Thanks,
Jan Schlicht
Re: Review Request 37188: Added std::hash template specializations.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37188/#review94517
-----------------------------------------------------------
Ship it!
3rdparty/libprocess/src/tests/http_tests.cpp (lines 677 - 687)
<https://reviews.apache.org/r/37188/#comment149141>
Can you explain why these two comparisons in each case need to be done.
- Alexander Rojas
On Aug. 7, 2015, 11:45 a.m., Jan Schlicht wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37188/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2015, 11:45 a.m.)
>
>
> Review request for mesos, Alexander Rojas and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added std::hash template specializations.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/address.hpp be216db823160f5db1dfb4502bf832246fb3df6d
> 3rdparty/libprocess/include/process/pid.hpp 3bce0bc99e0ebe3ac06ba53155d558fb041cd76c
> 3rdparty/libprocess/src/tests/http_tests.cpp ecbcbd552ac834659860627c82628ed38e6139b3
>
> Diff: https://reviews.apache.org/r/37188/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Jan Schlicht
>
>