Boost.URL is ACCEPTED into boost without any conditions. The review recommendations were 6 ACCEPT, 1 CONDITIONAL ACCEPT and 1 REJECT. Many points were raised as part of the review, based on which I recommend doing the following: - reviewing which types/functions in the lower-level API are exposed and which ones are not (Ruben Perez' condition) - reviewing pct_encoded_view & encoded_path (as mentioned by Дмитрий) - adding more verbose docs without relying on knowledge of RFC3986 on the user side (mentioned by Richard) - adding more simple examples (recommended by Jon Kalb) - consider adding IRI support in the future (recommended by Rainer) Alan & Vinnie have been very responsive and read all the reviews, which makes it unnecessary to summarize everything. There is only one real point of controversy, namely the only reason for rejection written by Phil Endecott:
More fundamentally, I'm not convinced that this allocation-avoiding view-rich API style is best for a general purpose vocabulary type like URL. URL should be a "Keep It Simple" type, accessible to C++ beginners, with straightforward lifetime semantics. Compare with std::filesystem::path, which does not try to optimise by actually being a view.
Because of these concerns about the basic API design, I believe that the library should be REJECTED.
This argument has however been directly addressed in other reviews, with Seth considering it as an appropriate choice:
Like some of the earlier reviewers, I also have some concerns over introducing non-owning vocabulary type. I do appreciate the design choice though, because I think it ultimately fits the Boost ecosystem best - favoring performance/versatility over ease of use.
While Zach Laine considers this as the correct choice:
Overall, I quite like the design. Lazy, lazy, lazy. The fact that only views into the underlying sequence are returned from the parse is great, and the fact that there are both owning (url::url and fixed-capcity owning url::static_url) and view versions of the URL representations is exactly what I would need as a user.
For my decision, the first factor is that more people approved of this particular design choice than disapproved. Secondly it seems to me that the C++ ecosystem is moving towards more view-support. The potential pitfalls can be addressed by coding guidelines (e.g. don't use auto) and do not need to be addressed by constraining libraries. Thus in finding a result, I did not consider Phil's argument to outweigh the other reviews which were considering it an acceptable choice. Other issues were the documentation, the confusion about encoded and decoded data and IRI support. Neither did lead to a rejection, so I'll leave it up to Alan's & Vinnie's discretion to review & address all the raised issues and to engage in further discussion on the mailing lists. Thank you Dmitry, Jon, Phil, Rainer, Richard, Ruben, Seth and Zach for your reviews and Alan & Vinnie for providing yet another boost library.
Other issues were the documentation, the confusion about encoded and decoded data
We are tracking these here:
https://github.com/CPPAlliance/url/issues/438
https://github.com/CPPAlliance/url/issues/448
and, in case anyone wants to contribute, these are the new functions, and
types we are considering:
pct_encoded_view -> decode_view
pct_decode_opts -> decode_opts
validate_pct_encoding -> (remove)
pct_decode_bytes_unchecked -> (remove)
pct_decode_unchecked -> decode_unchecked (private)
pct_decode becomes a single function to validate and decode ->
template< class CharSet = grammar::all_chars_t >
auto
decode(
string_view s,
CharSet const& allowed = {},
decode_opts const& opt = {}) ->
result< decode_view >
The functions for encoding are only renamed:
pct_encode_opts -> encode_opts
pct_encode_bytes -> encoded_size
template <class CharSet>
std::size_t
encoded_size(
string_view s,
CharSet const& allowed,
encode_opts const& opt = {}) noexcept;
pct_encode -> encode
template <class CharSet>
std::size_t
encode(
char* dest,
char const* end,
string_view s,
CharSet const& allowed,
encode_opts const& opt = {});
pct_encode_to_string -> encode_to_string
template<
class CharSet,
class Allocator =
std::allocator<char> >
std::basic_string
participants (2)
-
Alan de Freitas
-
Klemens Morgenstern