mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Joris Van Remoortere" <joris.van.remoort...@gmail.com>
Subject Re: Review Request 29533: Added a URL abstraction to stout.
Date Thu, 22 Jan 2015 23:28:51 GMT

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



3rdparty/libprocess/3rdparty/stout/include/stout/url.hpp
<https://reviews.apache.org/r/29533/#comment113935>

    Same comment as previous review: can we use a typedef / using declaration for this? It's
used quite a bit and would make refactoring easier.



3rdparty/libprocess/3rdparty/stout/include/stout/url.hpp
<https://reviews.apache.org/r/29533/#comment113936>

    I think all these fields can be const. There doesn't seem to be any mutation and they
are all assigned in the initializer list of the constructor.
    
    Are you expecting to allow implicit copies?



3rdparty/libprocess/3rdparty/stout/include/stout/url.hpp
<https://reviews.apache.org/r/29533/#comment113937>

    Add a TODO: use unrestricted union to collapse domain / ip, as only 1 can have a value.
At this point we can also get rid of the Option aspect, as one of the 2 is required. This
will make the restriction more clear.



3rdparty/libprocess/3rdparty/stout/include/stout/url.hpp
<https://reviews.apache.org/r/29533/#comment113938>

    Do we have a policy around using single quotes for single character literals? ':' instead
of ":"



3rdparty/libprocess/3rdparty/stout/include/stout/url.hpp
<https://reviews.apache.org/r/29533/#comment113942>

    Is there any less painful way to do this? It seems rather round-about / in-efficient.
(Constructing a vector of strings, then a giant string, and then appending that to a stringstream.
    
    I know we're trying to cleanly truncate one '&' character.
    
    For example, if we had an overload for std::pair<T, T> in stringify, then we could
pass the hashmap to the string::join('&', url.query) function.
    
    We might also want to support a strings::join for iterable that appends to the stream
(like the templated version does) as opposed to constring a string and then streaming that
out again.


- Joris Van Remoortere


On Jan. 21, 2015, 5:44 p.m., Benjamin Hindman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29533/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2015, 5:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> See summary.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/Makefile.am 8e51957d141af0be64cac42f65e03bca5929c8a9

>   3rdparty/libprocess/3rdparty/stout/include/stout/url.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/url_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29533/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message