Support for C++17 std::string_view in C++ wrapper

[NexAdn]

I just noticed that using std::string_view is not very convenient when working with liblo's C++ wrapper as it contains its own string_type which has no support for std::string_view. One needs to pass std::string_view::data() instead of the actual std::string_view to any function requiring a string_type.

The best option might be to replace string_type with std::string_view altogether, but this might introduce either the need to disable support for C++14 and below or to introduce an interface to string_type which is similar to std::string_view. The latter option might be better as the required interface needed seems to be only ::data() returning a const char*. In that case, it should be sufficient to check for the C++ version and use either using string_type = std::string_view or the own implementation.

The final option, which is probably the easiest to implement, but (in my opinion) the worst of the all, is to just accept std::string_view as parameter in string_type's constructor. This is, however, kinda like reinventing the wheel as std::string_view and string_type kind of do the same.

I might find some time implementing this myself and providing a PR, but in the meantime, feel free to add your opinion to this. If this is not a wanted enhancement, please let me know.

[radarsat1]

I've never really used string_view myself so I'll have to experiment a bit, but it's a nice idea. ;) The reason for the custom string type was to seamlessly support conversion from integers to strings (for specifying port in Address constructor) as well as to accept C-style and C++ strings, of course, without having to override each one. The natural thing in that case would be your last suggestion, to just accept string_view as an additional conversion. However, if that's not a desirable approach, I would look into at least what is the "normal" way for a C++17 function to accept both std::string or std::string_view, and move to that.

(Although technically liblo doesn't require C++17 yet.. it would be nice to be backward compatible to C++11 but hey.)

I don't know if C++17 might have more convenient ways to deal with number to string conversion, maybe some kind of std::variant, or a template approach, could be better here.

After a short search, I'm not clear on how to properly support string_view. It seems like functions need to be changed to accept string_view and then they will seamlessly accept string as well, which sounds ideal, except: 1) not backward compatible to C++11 (minor, perhaps), and 2) it seems that string_view is not guaranteed to be NULL-terminated; this is a real problem, since all uses of strings in liblo are to pass const char* to C functions, which do assume NULL termination.

So, due to that last point, maybe forcing the user to convert to string is actually the only right thing to do?

[NexAdn]

However, if that's not a desirable approach, I would look into at least what is the "normal" way for a C++17 function to accept both std::string or std::string_view, and move to that.

Generally, one should use just std::string_view to pass parameters instead of const std::string&. This avoids allocating memory and copying the string in case the parameter the caller provides is a const char*. If you'd like to accept both const std::string& and std::string_view, using std::string_view only is the way to go. If you then pass an std::string& as argument, it's converted to an std::string_view (i.e. a new std::string_view is created containing a pointer to the std::string's data and its length). Also, std::string_view has the advantage that it is easy to create a substring of another string using it. This might come in handy for some line-based configuration formats where only a portion of the current line needs to be passed to liblo (I'm thinking of something like configuring the custom behavior of certain OSC paths, like e.g. setting custom on/off colors on some controllers with RGB buttons).

I don't know if C++17 might have more convenient ways to deal with number to string conversion, maybe some kind of
std::variant, or a template approach, could be better here.

Well, there is std::to_string(T) which can take several standard types T (especially number types) and convert them into a string. No need to use stringstream here just to convert stuff. Maybe we can just add a template constructor which has template specializations for const char*, const std::string& (or similar) and std::string_view and a generic constructor just calling std::to_string on whatever is passed as parameter. Something like:

class string_type
{
    template <typename T>
    string_type(const T v) : string_type(std::to_string(v))
    {}

    template <>
    string_type(const std::string& s);

    template <>
    string_type(const char* s);

    string_type(std::string_view);
};

1. not backward compatible to C++11 (minor, perhaps),

Yeah, this issue could be solved by providing a custom string_type when dealing with C++ standards below 17. One could use the existing string_type, but there would be the need to adjust the interface slightly.

and 2) it seems that string_view is not guaranteed to be NULL-terminated; this is a real problem, since all uses of strings in liblo are to pass const char* to C functions, which do assume NULL termination.

Good point. Really haven't had that in mind, to be honest. I've just did some research and passing a const char* to std::string_view seems to guarantee that data() is not null-teminated (cppreference, constructor (4)). This disqualifies std::string_view as replacement for string_type completely.

So, due to that last point, maybe forcing the user to convert to string is actually the only right thing to do?

Not really, at least not if the user passes const char* as an argument. In that case, creating a std::string is just some extra overhead. That's what std::string_view tries to avoid.

---

Well, now that I know that std::string_view provides no method to obtain a null-terminated string, a const std::string& is still the better option. However, I would suggest at least implementing a constructor for string_type accepting an std::string_view. A std::string_view can be converted to std::string explicitly using a static_cast so this might be a viable option to obtain a null-terminated string from a std::string_view, although it is not the most elegant solution. This also keeps the overhead of keeping the C++ wrapper compatible to below C++17 minimal, as we just have to add one include and one constructor in case we are dealing with C++17 or above.

[radarsat1]

I played around a bit with trying to convert string_view seamlessly using a template, but unfortunately it did not convert easily with std::to_string, and if I added a very general template to string_type it messes up some polymorphism with send(). Finally I settled on your last idea, of just adding a constructor for string_view directly, and I used simply ifdef __cplusplus to check for C++17. Also, I added a null-termination check and it converts to std::string if not, otherwise it just passes the pointer through from data().

What do you think of this approach?

https://github.com/radarsat1/liblo/tree/add-support-for-string_view

[NexAdn]

The approach looks fine to me. Additionally, num_string_type seems to have become obsolete in this scenario. You could probably just move the (int v) constructor to string_type (or even make it a template to support any value which can be converted using std::to_string()). But that's just a could-do with the only obvious advantage having one class less.

[radarsat1]

Okay, yeah I'll test out removing num_string_type, but not sure it'll work due to the ambiguity it introduces for send(). I think the main role of num_string_type being distinct from string_type is actually to provide string_type as specific to strings, and not convert numbers when it's not intended. Although maybe there could be an alternative to string_type for the purpose of send(), such as using a template and to_string() instead. The idea there was mainly to avoid having to write separate functions for std::string and const char*. I'll try some things.

[radarsat1]

I noticed some changes related to this issue were included in the 0.32 release but they are from 4 years ago, so not confident whether this issue was resolved or still being worked on.