On Sun, Aug 21, 2022 at 4:03 PM Zach Laine <whatwasthataddress@gmail.com> wrote:
...
It wouldn't be a proper review without at least one person forgetting to use "Reply All" :) So here it is, from Zach. I have quoted his message:
This makes sense. The larger issue I'm raising is that a user is going to have a harder time remembering this, if the names don't match. A way to mitigate that is to make this naming rationale explicit, so that user expectations will adjust. I didn't get that this was the pattern from the documentation, though I think you did say it. A call-out specifically noting this would porbably be enough.
Yeah we can do that.
Hm. By lazy flat_map, do you mean that the positions are computed one step at a time during iteration? is that why they have forward_iterators?
Yes. operator++ and operator-- intelligently find the next param or segment using a non-validating parse algorithm (since we know that the underlying character buffer has already been validated).
If so, please document that. It might make a pretty big difference whether I decided to keep using a params object that I need to update a lot, or copy everything over to a proper flat_map or vector to do my work.
Yes I agree, but I thought we did document that. Here, no? <https://master.url.cpp.al/url/ref/boost__urls__params_encoded_view.html#url.ref.boost__urls__params_encoded_view.complexity> We could probably add something here too: <https://master.url.cpp.al/url/ref/boost__urls__params_encoded_view/iterator.html>
This works for an immutable container. It is *very surprising* for a mutable one. The rest of my notes about the containers are predicated on a false assumption that mutability implied non-laziness in the container itself. I think all of these container would be better off referred to as views, and made immutable. All you really need to support is begin/end, plus maybe key-lookup. I suggest you drop the mutability entirely. With mutability, these containers are trying to do two things at once.
Well, that's another way to do it and it certainly would remove complexity and simplify the names. Right now we have 8 containers: segments segments_view segments_encoded segments_encoded_view params params_view params_encoded params_encoded_view All of these containers reference the underlying character buffer of the URL (although you can also construct the views from separately parsed string views but that's basically the same thing in terms of the ownership model being reference-like). If we remove mutability as you suggest, then we can cut the number of containers down to 4: params_view segments_view params_encoded_view segments_encoded_view But this leaves us with a problem - how do you modify individual path segments and query params? How do you do position-based insert and erase of elements using an iterator? These features seem kind of useful, I'm not sure I want to lose them.
"Remove" is not really a term used in the STL. I think your "remove*()" container members should be "erase*()" instead.
I'm not sure where you're seeing this? There are no "remove" functions in these containers.
params::remove_value() is one. I think(?) there are others.
Oh. Well... remove_value() only removes the value part of the param. It does not erase the entire param. There's no such thing as insert_value. The javadoc is wrong unfortunately (sorry about that). We do use the term erase() when the operation removes the entire element.
Did you mean that it "doesn't" have proper error reporting? Where do you see this lack of reporting (if that's what you meant)?
By "proper" I mean that if the parse fails it should provide an end-user-friendly diagnostic, and optionally quote the line and put a caret under the position that the parse failed (a la the Clang diagnostics). Your non-end-user error reporting is fine.
Oh... hmm.. well, each rule does leave the iterator out-param at the spot where an error occurred. But I'm not quite sure how to emit the diagnostic - surely you aren't suggesting we write to std::cout or std::cerr? Are we using curses or something? Or maybe just plain ASCII art? How exactly would this look? Thanks to Peter's work on boost::system::error_code, when a parse error occurs the error is augmented with file name, line, and function name information. And I have worked on adding visualizers so these are nicely presented in the debugger. So at least we have that. Thanks