-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Proposal / discussion: Build all of coturn as C++17, instead of C11. #1416
base: master
Are you sure you want to change the base?
Conversation
@@ -81,8 +69,6 @@ void prom_set_finished_traffic(const char *realm, const char *user, unsigned lon | |||
void prom_inc_allocation(SOCKET_TYPE type); | |||
void prom_dec_allocation(SOCKET_TYPE type); | |||
|
|||
#endif /* TURN_NO_PROMETHEUS */ | |||
|
|||
#ifdef __cplusplus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the closing brace for extern "C" {
was mismatched in this file.
@@ -567,7 +567,7 @@ int stun_is_challenge_response_str(const uint8_t *buf, size_t len, int *err_code | |||
const uint8_t *value = stun_attr_get_value(sar); | |||
if (value) { | |||
size_t vlen = (size_t)stun_attr_get_len(sar); | |||
vlen = min(vlen, STUN_MAX_REALM_SIZE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C++ is a tad bit more picky about function arguments to std::min
, so casting to size_t (to match vlen
) fixes the compile error without any change in behavior.
src/apps/common/ns_turn_utils.c
Outdated
"LOG_LOCAL1", "LOG_LOCAL2", "LOG_LOCAL3", "LOG_LOCAL4", "LOG_LOCAL5", | ||
"LOG_LOCAL6", "LOG_LOCAL7", "LOG_LPR", "LOG_MAIL", "LOG_NEWS", | ||
"LOG_USER", "LOG_UUCP", "LOG_AUTHPRIV", "LOG_SYSLOG", 0}; | ||
static char const *const str_fac[] = {"LOG_AUTH", "LOG_CRON", "LOG_DAEMON", "LOG_KERN", "LOG_LOCAL0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined behavior to assign char*
variables with string literals.
In both C and also C++.
Just C-language will let you do it anyway, and C++ won't (without a compiler flag that looks scary).
|
I'm not going to use a 10 year old, or older, version of the language. That's completely out of the question. I'm also not interested in writing a new implementation from scratch. My objective is to keep the same architecture, and largely the same code. My main interest in using c++ is string handling ( |
Then I think it's better to keep C!It is not advisable to mix C++ ( |
Can you provide some explanation of why you think it is not advisable? |
To follow |
4ef2d33
to
930f0b5
Compare
d9ab538
to
7fdd52d
Compare
1b141c3
to
af7a083
Compare
Resolves #1389
Currently
coturn
is compiled with each source file treated as C-language files, specifically C11 (though, the adoption of features from C11 is not very far along, e.g. stdbool.h).This pull request changes the compilation settings of both the Makefile based, and CMake based builds of
coturn
to instead treat the source files as C++17.Note: In the linked issue I asked about patches to support compiling as C++ instead of actually compiling as C++. This PR was so easy to put together I figured I'd skip straight to feeling the waters on full adoption before bothering only submitting the minor changes needed support "Optionally, but not always, allow compiling as C++". If the idea of switching to C++ is outright rejected, I'll still want to put in the compile-as-C++ fixes, but would strongly prefer we just go for it.
Note: This specific PR does not rename the C files to have the
.cpp
extension, which would cause CMake to implicitly assume they are C++ files, because I did not want to make the diff crazy looking. If the maintainers ofcoturn
are interested in adopting C++ instead of C-language, then it would be less messy to rename the existing files instead of instructing CMake to treat the files as C++.Motivation
coturn
project to adopt C++ specific functionality that it does not want, but compiling as C++ is a prerequisite for being able to use any C++ features at all.I'll put a break in the list here to emphasis this much more strongly.
I am NOT saying
I AM saying
coturn
when writing in a language that I am familiar with.coturn
), I anticipate spending a substantial amount of time working withcoturn
, and... to be frank and perfectly honest, I really don't like C. This change will make me more effective and happier to help maintain the project.coturn
, including the two most recent CVEs that I'm aware of, have to do with string handling, and the allocation, freeing, and pointer management thereof.__attribute__((cleanup))
(see https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html ).constexpr
andinline
variables and functions, which can help reduce theextern
and#define
soup that most of the coturn internal headers appear to be right now, while also providing the compiler better opportunities to optimize the resulting code.If nothing else, the C++ Destructors is the critical functionality that would improve the
coturn
project's security posture the most.I would be happy to see
coturn
switch to C++ EVEN IF IT DOES NOT USE ANYTHING BUT C-language + C++Destructors.I'll stop listing motivations there, even though I could probably write many pages of benefits.
Instead I'll write a bit more about what C++ will let me do in the short-term.
Followup after switching to C++
turnutils_uclient
program, with the majority of functionality stripped out and some new behavior added. Switchingcoturn
to C++ will let me submit a bunch of improvements toturnutils_uclient
before forking it and removing the unnecessary bits (and adding the new bits)char*
/char const*
with std::string.Those are all things that I have the flexibility to implement in the short-term and make PRs for. I don't have unlimited time, but I have several weeks where coturn is my primary focus.
FAQ
coturn
libraries (are there any...? I seeclient
, but i'm not sure what even uses that?) would have to change. The public interface can remain C-language code without interfering with the implementation files compilation language. This isn't really even more or less work, as the C-language public interface already exists, and any C++-isms can be exposed to the world as "also" instead of "instead of".coturn
adopt C++templates. While there might be some place in thecoturn
code that could benefit from templates, I have not yet encountered it.coturn
are running on Linux, Windows, Mac, BSD, or Solaris, and out of those 5, probably only Linux has anywhere close to over %1 of the users.coturn
as a project should attempt to support platforms that are so esoteric that the GCC project doesn't have a build for it. Those users are perfectly capable of continuing to use the version ofcoturn
that they're using now, and fixes could be applied to a long-term-service branch of this repo to keep those users on life-support if any of them ever come asking.