----------------------------------------------------------- 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 > >
