Add iterator support to url_search_params

[jasnell]

Does two things:

• Adds simple iterators to ada::url_search_params. These are implemented as distinct types with next() and is_done() methods as opposed to traditional c++ iterators.

• Adds url search params apis to ada_c

[anonrig]

@jasnell Can you add documentation to each function and a small example to README?

[anonrig]

Looks good to me. I want to take a look at it after @lemire leaves a review. Thank you @jasnell

[lemire]

@jasnell Looks fine to me.

However, I am unsure why you don't want to use conventional C++ iterators, which allow you to automagically do this:

 for (auto key : search_params.get_keys()) {
   // do something with key
 }

The approach you document...

auto keys = search_params.get_keys();
 while (!keys.is_done()) {
   auto key = keys.next();  // "a", "c", "e", "g"
 }

will appear odd (I don't mean to be insulting) to many modern C++ users. It is also objectively 'busier' and easier to get wrong.

Furthermore, why 'is_done' instead of the conventional 'has_next... That is, the following is easier to understand...

auto keys = search_params.get_keys();
 while (keys.has_next()) {
   auto key = keys.next();  // "a", "c", "e", "g"
 }

Isn't it?

Can you elaborate on your design decisions ?

[lemire]

Effectively, I think you need very strong arguments if you are going to bypass the standard C++ iterators. They are easy to implement, and they lead to all sorts of goodies. They are expected in C++.

[jasnell]

Yep, I'd considered implementing normal c++ iterators but opted for this model to keep it conceptually similar to JavaScripts iterator pattern (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols#the_iterator_protocol), which has next(), done, etc. The key expectation being that these are most likely to be wrapped in an implementation of the URLSearchParams class where next() is called to advance.

There the pattern for wrapping these is more likely something like (forgive the mild pseudocode)

class URLStringParamsKeysIterator {
 public:
  struct Result {
    bool done;
    JsValue value;
  };
  Result next() {
    if (iter.done) return { true, JsUndefined};
    return { false, iter.next() };
  }

private:
  ada::url_search_params_keys_iter iter;
}

I wanted also to leave room for someone to implement the conventional c++ iterator pattern if they wanted -- these should be able to co-exist. I just really have no immediate need for the c++ style iterators. I need this only to support the JS pattern.

And yes, I know that I could implement these using the C++ iterator pattern under the covers but this was just easier :-)

Also note that the JS iterator pattern for URLSearchParams is... quirky, when it comes to edits. The iterator itself needs to continue to work in the face of edits, even if the behavior is... odd. If I was implementing a conventional c++ iterator I would want the behavior to be a bit stricter here.

[jasnell]

@lemire @anonrig ... ok, I've updated to add the conventional c++ iterator in addition to the JS-style. For the c++ iterator I'm really just deferring to the underlying std::vector for entries, whereas the js-style has distinct instances for keys, values, and entries.

I also renamed is_done to has_next and updated accordingly. Hopefully this will strike a nice balance for both JS and C++ use.

[lemire]

Approved.

[jasnell]

I see that many of the operations in ada_c are intended to be safe when !r but the current approach means that these will crash after the various ada_free* calls. In a separate PR I'll go through and make these all safer (not just the search_params ones) so that double frees and the other ops are more resilient after free.