Re: boost.url review

From: Alan de Freitas (alandefreitas_at_[hidden])
Date: 2022-08-22 17:36:02

Hi, everyone.

This is a summary of issues we have opened during the review so anyone
interested can track them.

* Public grammar namespace:

https://github.com/CPPAlliance/url/issues/444

I find the creation of a mini-parsing library and the expectation that
> users will use this mini-library to write their own parsers to be
> somewhat problematic.

Regarding the parser interface, I feel this should mature into its own
> library.

I'm not very keen on exposing the parsing infrastructure and helper
> functions

I can see other public types in the reference such as recycled,
> recycled_ptr and aligned_storage, which again have the feeling
> should not be exposed as public types.

* parse_url vs. parse_uri:

https://github.com/CPPAlliance/url/issues/443

When I call parse_uri(), I get a result of type result<url_view>? Why
> isn't the function spelled parse_url()

In contrast to others, I think the choice of Nomenclature, namely URL
> everywhere except in the grammar domain (the parse function names) is a
> very
> elegant one

* Documentation issues:

https://github.com/CPPAlliance/url/issues/442

(About 20 minor issues with the docs)

* Exploring IRIs:

https://github.com/CPPAlliance/url/issues/441

The lack of IRI support is unfortunate. It's 2022; we should all
> be writing software with Unicode support by default. However, this can
> be built on top of Boost.URL, and isn't needed in all cases.

* Add lowercase/uppercase in pct_encode_opts:

https://github.com/CPPAlliance/url/issues/440

In the past I have had situations where it was necessary to specify the
> case for hex letters

 * Optional query values:

https://github.com/CPPAlliance/url/issues/439

I would be inclined to use a std::optional for the value instead.

* Name of the decoding view is confusing:

https://github.com/CPPAlliance/url/issues/438

I would expect a "decoded_view" to refer to underlying encoded data

pct_encoded_view is an unfortunate choice for a view that actively decodes
> URL-encoded characters

I did also find the naming situation for encoded/decoded a bit confusing. I
> would expect the opposite.

* Using std::string in url:

https://github.com/CPPAlliance/url/issues/437

In your non-view url object, you seem to allocate storage using new char[].
> Why not use a std::string?

* url::persist docs:

https://github.com/CPPAlliance/url/issues/436

so url_view doesn't own the underlying storage, except when it
> does as a result of this mysterious method. What's the rationale for
> this?

* url lifetimes:

 https://github.com/CPPAlliance/url/issues/435

There's no indication here that r is a non-owning view and that
> the caller is responsible for keeping s alive for the lifetime of r.

* Examples:

 https://github.com/CPPAlliance/url/issues/419

I miss an example (and possibly a page) about composing and mutating URLs

I'd add a page and an example on percent encoding functions

small examples may be even more useful to newcomers.

I found resolve a bit mystifying. (...) it will be a big help to have
> inspirational examples

Perhaps the examples would be better when fleshed out in long form

* pct-encoding overloads:

 https://github.com/CPPAlliance/url/issues/417

 I'd consider exposing higher-level functions here, maybe taking a lvalue
> reference to a std::string

* container names:

https://github.com/CPPAlliance/url/issues/416

I've found the naming for view containers confusing (e.g. params and
> params_view).

I would suggest leaving url and url_view as-is, and replacing the "view"
> part for "const" for the rest of the containers

Em sex., 12 de ago. de 2022 Ã s 23:30, Klemens Morgenstern via Boost <
boost_at_[hidden]> escreveu:

> Hi all,
>
> the formal boost review of the boost.url library starts today, the 13th of
> August and will last until and including the 22nd of this month.
>
> The library has been developed by Vinnie Falco and Alan de Freitas.
> The master branch is frozen during the review and can be found here:
> https://github.com/CPPAlliance/url
>
> The current documentation can be found here: https://master.url.cpp.al/
>
> Boost.URL is a portable C++ library which provides containers and
> algorithms which model a "URL" and understands the various grammars related
> to URLs and provides for validating and parsing of strings, manipulation of
> URL strings, and algorithms operating on URLs such as normalization and
> resolution.
>
> Please explicitly state that you either *accept* or *reject* the inclusion
> of this library into boost.
> Also please indicate the time & effort spent on the evaluation and give the
> reasons for your decision.
>
> Some questions to consider:
>
>  - Does this library bring real benefit to C++ developers for real world
> use-case?
>  - Do you have an application for this library?
>  - Does the API match with current best practices?
>  - Is the documentation helpful and clear?
>  - Did you try to use it? What problems or surprises did you encounter?
>  - What is your evaluation of the implementation?
>
> More information about the Boost Formal Review Process can be found
> at: http://www.boost.org/community/reviews.html
>
> The review is open to anyone who is prepared to put in the work of
> evaluating and reviewing the library. Prior experience in contributing to
> Boost reviews is not a requirement.
>
> Reviews can also be submitted privately to me, and I will not disclose who
> sent them.
> I might however cite passages from those reviews in my conclusion.
>
> Thanks to Vinnie & Alan for providing us with a new library & thanks for
> all the reviews in advance.
>
> Klemens
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

-- 
Alan Freitas
https://github.com/alandefreitas