You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Abhishek Dasgupta <a1...@linux.vnet.ibm.com> on 2016/01/26 11:55:02 UTC
Review Request 42794: URL query string order is defined.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42794/
-----------------------------------------------------------
Review request for mesos, Jan Schlicht and Vinod Kone.
Bugs: MESOS-3317
https://issues.apache.org/jira/browse/MESOS-3317
Repository: mesos
Description
-------
URL query string order is defined.
Diffs
-----
3rdparty/libprocess/src/http.cpp 762da9a9038fc0a81156b5c03b556084df1bd7e0
3rdparty/libprocess/src/tests/http_tests.cpp 66d185e3fd8a165d8a98d3d2752e756f8de3499b
Diff: https://reviews.apache.org/r/42794/diff/
Testing
-------
The following test cases in http_test.cpp have been modified -
HTTPTest.QueryEncodeDecode to check generated query strings in URL to be according alphabatic order.
URLTest.Stringification to check generated query strings in URL to be according alphabatic order.
Thanks,
Abhishek Dasgupta
Re: Review Request 42794: URL query string order is defined.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42794/#review116362
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (line 787)
<https://reviews.apache.org/r/42794/#comment177405>
Renaming this to `sortedQuery` better reflects the property of this map.
3rdparty/libprocess/src/http.cpp (line 788)
<https://reviews.apache.org/r/42794/#comment177406>
The size of `query` is know here, please reserve it for `_query`:
```cpp
_query.reserve(query.size());
```
3rdparty/libprocess/src/http.cpp (lines 789 - 791)
<https://reviews.apache.org/r/42794/#comment177404>
Please add a comment above this statement to explain what's happening here. I.e. "Adding `query` to a map will sort them alphabetically by their fields".
3rdparty/libprocess/src/http.cpp (line 790)
<https://reviews.apache.org/r/42794/#comment177407>
Why not use `map::emplace` here, to avoid unnecessary copies?
- Jan Schlicht
On Jan. 26, 2016, 11:55 a.m., Abhishek Dasgupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42794/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 11:55 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-3317
> https://issues.apache.org/jira/browse/MESOS-3317
>
>
> Repository: mesos
>
>
> Description
> -------
>
> URL query string order is defined.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 762da9a9038fc0a81156b5c03b556084df1bd7e0
> 3rdparty/libprocess/src/tests/http_tests.cpp 66d185e3fd8a165d8a98d3d2752e756f8de3499b
>
> Diff: https://reviews.apache.org/r/42794/diff/
>
>
> Testing
> -------
>
> The following test cases in http_test.cpp have been modified -
> HTTPTest.QueryEncodeDecode to check generated query strings in URL to be according alphabatic order.
> URLTest.Stringification to check generated query strings in URL to be according alphabatic order.
>
>
> Thanks,
>
> Abhishek Dasgupta
>
>
Re: Review Request 42794: URL query string order is defined.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42794/#review116355
-----------------------------------------------------------
Patch looks great!
Reviews applied: [42794]
Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export COMPILER=gcc; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 26, 2016, 10:55 a.m., Abhishek Dasgupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42794/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 10:55 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-3317
> https://issues.apache.org/jira/browse/MESOS-3317
>
>
> Repository: mesos
>
>
> Description
> -------
>
> URL query string order is defined.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 762da9a9038fc0a81156b5c03b556084df1bd7e0
> 3rdparty/libprocess/src/tests/http_tests.cpp 66d185e3fd8a165d8a98d3d2752e756f8de3499b
>
> Diff: https://reviews.apache.org/r/42794/diff/
>
>
> Testing
> -------
>
> The following test cases in http_test.cpp have been modified -
> HTTPTest.QueryEncodeDecode to check generated query strings in URL to be according alphabatic order.
> URLTest.Stringification to check generated query strings in URL to be according alphabatic order.
>
>
> Thanks,
>
> Abhishek Dasgupta
>
>
Re: Review Request 42794: URL query string order is defined.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42794/#review116392
-----------------------------------------------------------
3rdparty/libprocess/src/http.cpp (lines 787 - 791)
<https://reviews.apache.org/r/42794/#comment177428>
Instead of manually iterating, just create the `map` directly from the `hashmap` values, e.g.,
const map<string, string> _query{query.begin(), query.end()};
(rename `_query` as you see fit). This removes most of the issues Jan raised.
- Benjamin Bannier
On Jan. 26, 2016, 11:55 a.m., Abhishek Dasgupta wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42794/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2016, 11:55 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-3317
> https://issues.apache.org/jira/browse/MESOS-3317
>
>
> Repository: mesos
>
>
> Description
> -------
>
> URL query string order is defined.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/src/http.cpp 762da9a9038fc0a81156b5c03b556084df1bd7e0
> 3rdparty/libprocess/src/tests/http_tests.cpp 66d185e3fd8a165d8a98d3d2752e756f8de3499b
>
> Diff: https://reviews.apache.org/r/42794/diff/
>
>
> Testing
> -------
>
> The following test cases in http_test.cpp have been modified -
> HTTPTest.QueryEncodeDecode to check generated query strings in URL to be according alphabatic order.
> URLTest.Stringification to check generated query strings in URL to be according alphabatic order.
>
>
> Thanks,
>
> Abhishek Dasgupta
>
>