Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No specified C++ standard in make files and potential issues #103

Closed
GiuseppeCesarano opened this issue May 21, 2023 · 6 comments
Closed

Comments

@GiuseppeCesarano
Copy link
Contributor

GiuseppeCesarano commented May 21, 2023

The make files responsible for compiling the C++ examples in the project currently lack a selected C++ standard, and there is no mention of the targeted C++ standard within the project.

This could lead to:

  1. Lack of feature awareness for contributors
    Contributors may face difficulties in determining which C++ language features are available to them. For instance, the current implementation of the window::return_string member function in the C++ bindings:
void return_string(webui::event* e, std::string s) {
  webui_event_t* c_e = convert_event_to_webui_event(e);
  webui_return_string(c_e, &s[0]);
  delete c_e;
}

This demonstrates passing a string by value, which can be computationally expensive. However, in this particular case, such copying is unnecessary, as the function webui_return_string already creates a copy of the string. To mitigate this, there are two main approaches: passing the std::string as const std::string& or utilizing std::string_view. The latter is the recommended approach due to its ability to alias to various types and potentially avoid costly calls to the std::string constructor. Nevertheless, adopting std::string_view requires C++20, and implementing this change with the current make files will result in compilation errors.

  1. Inconsistency across compilers
    By not explicitly defining the C++ standard, the compilers will resort to defaulting to a particular standard based on the compiler version. For instance, clang 15 will default to using C++14, while clang 16 will default to C++17. This inconsistency can lead to the infamous "it works on my PC" problem.

Additionally, considering the nature of the project and the potential benefits of modern C++ features, it is recommended to adopt at least C++20 as the targeted standard within the make files.

hassandraga added a commit that referenced this issue May 21, 2023
* Set C++11 as minimal standard
* Fix C++ Makefile in macOS
* Update the C++ WebUI header to use references (#103)
* Update `webui_return_string()` argument from `char*` to `const char*`
@hassandraga
Copy link
Member

Thank you, Giuseppe, for pointing this out.

Honestly, this project is purely C99, but someone on Reddit requested a C++ wrapper, so I created the C++ wrapper in 20-25 minutes in a rush... and for sure, it's not complete and needs improvements.

You are right about the unnecessary strings copy, so I update it to use references instead of values, especially when APIs take const char*. So, thank you for reporting that.

About the standard, the WebUI C++ wrapper is relatively small and easy, so selecting a minimum standard as C++11 is enough. This helps developers who are using WebUI in old embedded/PC devices. On the other hand, anyone can easily modify the webui.hpp and makefile to set a different standard as needed.

Fix: cdf67b1

@GiuseppeCesarano
Copy link
Contributor Author

This fixes the problem, so I'm closing the thread.

However, I want to point out that taking const std::string& as a parameter can be just as bad as taking std::string as a parameter, for example, in the worst case, if a string literal exceeding the length of 20 is passed to a function expecting a const std::string&, the std::string constructor is called and an allocation and copy occurs.

https://godbolt.org/z/n3vh9TzP7

@AlbertShown
Copy link
Contributor

That's true, but unfortunately, this is the nature of C++.
const std::string& is the best option take take std::string foo = "Hello" or "Hello". When you pass in an existing std::string object, there will be no copy or allocation, just a reference to the existing string. When you pass a string literal const char*, a temporary std::string object must be created (Copy).

The best solution is providing overloads for both const char* and std::string to handle both cases optimally.

@GiuseppeCesarano
Copy link
Contributor Author

Saying that is the nature of C++ does not feel right to me; I think is more the nature of std::string, this container is meant to own the underlying data and thus manage its lifetime, taking a const reference to a std::string is almost a betrayal of its intended use, you are saying "I just want to access the string, but do it troght a thing that should own it".

This is not a big deal, if I'm being honest, since the compiler will optimize the copy if the string literal can fit in the SSO, and the temporary object will be moved or emplaced instead of being copied.

But even though I'm a C++ guy, I think that a const char* is the better choice (if std::string_view is not aviable), first of all it is explicit about its intent as a non-owning type, and morever it can't trigger a copy, an allocation and a strlen call.

std::string also has a member function that returns const char*, std::string::c_str(), so this would mean that a user passing std::string would be required to call .c_str(), making the function call a bit more verbose.

The last option is to basically reimplement std::string_veiw in a way that can also be used in your C api, in my opinion this might be an option if you notice too many strlen calls, and it might also solve the problem of having to call c_str() or deal with the std::string constructor; obviously this is a time investment and might not be worth it.

Each of these ways of dealing with strings is valid in my opinion, and each way has it's downside and it's strengths; I'm just pointing out that strings are hard, and the API that deals with them should be carefully considered.

@AlbertShown
Copy link
Contributor

I already taught about using const char*, but as you said, it will make the user call .c_str(), making the code look strange!

Honestly, using std::string_view can be a good option in many cases, especially since we don't modify the string. Would you like please modify webui.hpp? I will modify Makefiles to use the C++17 standard instead of C++11.

@GiuseppeCesarano
Copy link
Contributor Author

I was already preparing a patch with some minor stuff. If you feel like doing the standard change I will add the string_view and wait for your commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants