Why is http_header_fields_t::find() private?

[rcane]

Is there a specific reason that all find methods in http_header_fields_t are private? If I want to check for the existence of a field and get its value if it exists, I have to either use has_field and get_field doing the lookup twice, or I could use the public begin and end methods with std::find_if duplicating what find already does.

[eao197]

It's hard to say why, because this code was written 2 years ago and we hardly find the right answer now.

From the current point of view, the API in http_header_fields_t is not good. I think it is better to have additional get_field versions, something like (updated):

optional_t<std::reference_wrapper<const std::string>>
get_opt_field(string_view_t field_name) const noexcept;

optional_t<std::reference_wrapper<const std::string>>
get_opt_field(http_field_t field_id) const noexcept;

And method for_each_field:

template<typename Lambda>
void for_each_field(Lambda && lambda) const noexcept;

instead of begin and end members.

[rcane]

I completely agree that this would be a much better API.

I didn't even know that optional was an option (no pun intended ;-)). That is what I would have needed.

Also, begin and end expose the container type to the outside making it hard change if it should become necessary.

[eao197]

I didn't even know that optional was an option

IIRC, optional was added sometime after the first versions of RESTinio were published. And because we target C++14, we have to use a great optinal_lite library for the implementation of optional.

[eao197]

It also seems that the current version of get_field(name, default_value) is dangerous. For example:

const auto & v = headers.get_field("Accept-Charset", "utf-8");

If "Accept-Charset" is not found then v will hold a dangling reference to destroyed temporary object.

I think this should be refactored too. Maybe that version of get_field should have that prototype:

std::string get_field(string_view_t name, const std::string & default_value) const;
std::string get_field(http_field_t field_id, const std::string & default_value) const;

Or this vesion of get_field should be deprecated and removed later. And new methods like that should be introduced:

template<typename Lambda>
decltype(auto) for_field_or(
      string_view_t name, const std::string & default_value, Lambda && lambda) const {
   const std::string * value = ...; // Search using find().
   if(!value) value = &default_value;
   return lambda(*value);
}

I personally prefer for_field_or, but it seems that this subject needs additional thinking.

[rcane]

Simply not returning a string reference from get_field would be my choice. It just fixes the problem without having to touch any client code.

Your for_field_or proposal is much more general and you can do a lot more with the lambda. So I think that this should be an extension to the simple-to-use get_field API but not replace it.

Otherwise the common use case of simply getting the value of a field would have to be written like this:

auto const value = for_field_or("Accept-Charset", "utf-8", [](auto const& value) { return value; })

This is a little too verbose for my taste. ;-)

[eao197]

@rcane Thanks for sharing your opinion.

[eao197]

I've just started refactoring of http_header_fields_t interface and it seems that this format of get_opt_field:

optional_t<std::reference_wrapper<const std::string>>
get_opt_field(string_view_t field_name) const noexcept;

it not a convenient. May be it is better to have that form:

optional_t<const std::string*>
get_opt_field(string_view_t field_name) const noexcept;

With this version the result of get_opt_field will be very similar to just const std::string **:

auto f = headers().get_opt_field("Content-Type");
if(f)
  std::cout << "Content-Type is: " << **f << std::endl;

[rcane]

For me, an optional<T*> is quite unusual. Much more so if T is a class type. As a return type it immediately brings up the question if T* can be nullptr and what it means if it actually is. In case of get_opt_field you would need to document that if the returned optional is valid the wrapped pointer will never be nullptr. So users would have to remember that. But even if we accept this, it is still strange to having to double-dereference the return value to get to the actual string. When I see things like **foo I always think that I am looking at C code. ;-)

Although using reference_wrapper inside the optional looks a little unusual I find it to be the cleaner solution because it is self-documenting (i.e. I know that it is valid). And more importantly I get to the value by simply de-referencing the optional.

[eao197]

And more importantly I get to the value by simply de-referencing the optional.

It's not that simple, I'm afraid. For example, with optional<T*> it is easy to write like that:

if(const auto f = headers().get_opt_field("Content-Type"); f && **f == "text/plain") {...}

But with reference_wrapper you'll get a compiler error in such case:

if(const auto f = headers().get_opt_field("Content-Type"); f && *f == "text/plain") {...}

[eao197]

Maybe the problem with nullability of optional_t<T*> can be solved with such simple typedef:

template<T>
using not_null_pointer_t = T*;

So we can return optional_t<not_null_pointer_t<const std::string>>.

[rcane]

That would make it more clear for the reader but the **f thing would still remain. But maybe there is no way around it.

Using std::reference_wrapper, your if example would need to be written like this:

if(const auto f = headers().get_opt_field("Content-Type"); f && f->get() == "text/plain") {...}

Apparently the compiler cannot automatically deduce that it should use reference_wrapper::operator T&.

Btw, this if example is C++17. I thought RESTinio was restricted to C++14.

[eao197]

I think there are (at least) three ways of changing the API:

1. The most simple one:

   const std::string * get_opt_field(string_view_t name) const noexcept;

   It's simple and powerful, but there is some bias towards raw pointers in modern C++. Moreover, when this method is used that way:

   const std::string * v = headers().get_opt_value("Content-Type");
   if(*v == "text/plain") {...} // (1)

   it's hard to say is the line (1) correct or not.

2. The version with optional_t<T*> or optional_t<not_null_pointer_t<const std::string>>. It is not as simple as the first one, but it is still simple. And is very similar to a double pointer.

3. The version with optional_t<std::reference_wrapper<const std::string>>. It's the most complex. It's not as convenient as two previous variants. It's more verbose. And (I suppose) it will have some troubles with code like that:

   const auto some_lambda = [](const auto & str) { // Note the usage of `auto` here.
   return str == "some_value";
   };
   const auto f = headers().get_opt_value("Content-Type");
   if(f && some_lambda(*f)) {...}

So I think that if I have to choose between those three variants I would prefer two firsts but not the last one.

The current version of the refactored API can be seen here. It uses optional_t<not_null_pointer_t<const std::string>>. But this can be changed, of course.

Btw, this if example is C++17. I thought RESTinio was restricted to C++14.

It was just an example. RESTinio uses C++14 for v.0.4.*. There is no plans to upgrade to C++17 for 0.4 branch.

[rcane]

What do you think about returning optional<string_view>? This works with your lambda example and would convey the intent of the return value quite nicely (at least for me).

In the end there won't be a perfect solution. I could certainly live with all proposed versions. Just because I don't like pointers does not mean that they are bad. ;-)

[eao197]

What do you think about returning optional?

It can be a better solution. I'll think about it.

It would be easier if get_field returns string_view_t instead of const std::string &. In that case, there will be a consistent API:

string_view_t get_field(string_view_t name) const;
optional_t<string_view_t> get_opt_field(string_view_t name) const noexcept;

But changing type of the return value of get_field is a serious break of compatibility.

Maybe it is better to introduce some new methods:

string_view_t value_of(string_view_t name) const;
string_view_t value_of(http_field_t field_id) const;
optional_t<string_view_t> opt_value_of(string_view_t name) const noexcept;
optional_t<string_view_t> opt_value_of(http_field_t field_id) const noexcept;

[rcane]

I am all for not breaking existing interfaces and value_of reads nice in a statement.

auto const v = header.value_of("Content-Type");

I like it.

[eao197]

There is another update. Now there are two groups of field's value getters. The first one is the old get_field and friends:

const std::string & get_field(string_view_t) const;
const std::string & get_field(http_field_t) const;
nullable_pointer_t<const std::string> try_get_field(string_view_t) const noexcept;
nullable_pointer_t<const std::string> try_get_field(http_field_t) const noexcept;
std::string get_field_or(string_view_t, default_value) const;
std::string get_field_or(http_field_t, default_value) const;

The second is new value_of methods with usage of string_view_t as result type:

string_view_t value_of(string_view_t) const;
string_view_t value_of(http_field_t) const;
optional_t<string_view_t> opt_value_of(string_view_t) const noexcept;
optional_t<string_view_t> opt_value_of(http_field_t) const noexcept;

I didn't add value_of_or (or something like that):

string_view_t value_of_or(string_view_t name, string_view_t default_value) const noexcept;

because there is a possibility to return a view to destroyed temporary object in some usecases.

[rcane]

I use the new opt_value_of in my code now and it works as expected.