[Discussion] C++ guidelines / tools

[Krzmbrzl]

As mentioned in #4195 (comment) we're sticking to C++ for the future of Mumble development.

We still want to make sure that we make the code as stable and as maintainable as possible and in order to achieve that we'd like to gather some guidelines / best practices here. Furthermore we're also looking for tools (like static analyzers) that we can deploy (on the CI) that ensure code quality as good as possible.

---

Guideline	Enforcement
Stay as close to the cpp core guidelines as possible	clang-tidy
Never user new and delete: We should completely switch to smart-pointers for managing and expressing ownership. In general we should only have one single owner of a given piece of data, so std::unique_ptr is probably the best choice in like 99% of cases. For allocating new objects we should then use std::make_unique	simple keyword check -> python script; clang-tidy?
Use raw pointers for function arguments to indicate data that must not be taken ownership of (e.g. only pass as smart-pointer if the function should take ownership) - still prefer (const) references over pointers where possible though	Code review (as it depends on the situation)
Use clang's thread safety analysis for making the intentions clear (e.g. use GUARDED_BY, REQUIRES, etc.) everywhere a lock is involved	clang-thread-safety-analysis
Make each function list the threads it may run in. That way a dev can look at the function and will immediately know which function it runs in. Maybe we can use static_assert for this?	? (maybe we can add a custom rule to clang-tidy?
Use (clang's) sanitizers in the CI when running tests	clang-sanitizers
Use proper unit/integration testing	Maybe mutation testing?
Make all copy-constructors private and supply clone() method instead - that way we could avoid unintentional copying	custom rule for clang-tidy?
Use const everywhere. Not only should we be const-correct but we should mark a value as const, if it doesn't change. In any situation.	?
Make use of move-semantics: Returning stuff by value (even for std::vector, etc.) is okay as we have move-semantics. No need to work with pointers for that	Code Review
Uniform formatting of the source-code	clang-formatter (+ some custom check on CI whether re-formatting changes a file's content)
Documentation of all functions & parameters	hyde and/or Doxygen (standardese?)
Comment the source-code with intention (not implementation)	Code review
No globals	?
No non-const public member fields	?

Links

• https://www.kdab.com/clang-tidy-part-1-modernize-source-code-using-c11c14/
• https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
• https://maitesin.github.io/clang_sanitizers/
• https://stackoverflow.com/questions/24609599/doxygen-display-warning-for-undocumented-method

---

Why am I putting so much value into enforcing the new guidelines? Well as an OpenSource application we have a wide variety of contributors and relying on every contributor to magically know about the rules and to also stick to them does just not work. Same applies to expecting a code-reviewer to never forget about checking one of these guidelines.
Therefore I think having a program to check all of these guidelines is actually the best way for us to make sure that no code slips through the review :)

---

If you have additional suggestions as to what we should aim for for our guidelines, please let us know. Also if you think one of the suggested ones is a bad idea, let us know why. And finally: If you know other / better tools for static analysis and/or enforcing one of the guidelines, please shar your knowledge here.

[felix91gr]

Gonna recommend clang static analysis tools. They tend to be pretty amazing.

And on dynamic analysis, the clang Sanitizers (address sanitizer, memory sanitizer, ubsantizer) are pretty amazing. They're not like valgrind (in the sense that valgrind is basically a VM). They work by compiling your code with some dynamic checks added, and seeing if while running, your memory/pointers/etc raises any flags.

edit: oh heck I didn't see the first comment. I might've been too early. Sorry about that! Gonna read it and maybe write more tomorrow (I'm again, awake at some ungodly hours...)

[felix91gr]

I see you've left a comment there on Mutation Testing.

I've been following the work on Mull for years, and the two guys making it are pretty legit and have shown some seriously cool results. It is C++, and therefore it basically works out of the box for C and Cpp :3

[felix91gr]

Why am I putting so much value into enforcing the new guidelines? Well as an OpenSource application we have a wide variety of contributors and relying on every contributor to magically know about the rules and to also stick to them does just not work. Same applies to expecting a code-reviewer to never forget about checking one of these guidelines.
Therefore I think having a program to check all of these guidelines is actually the best way for us to make sure that no code slips through the review :)

I super agree. Both making the rules explicit in the "contributing" documentation or whatever, and making CI check for formatting and e.g. code coverage or a mutation testing score, is work off from everyone's shoulders that will definitely show results in just a couple of months of usage. I think making this explicit and/or automatic is a fantastic idea :)

[bendem]

Documentation of all functions & parameters

Having had that enforced before, I would argue against it. It tends to mean documenting meaningless functions (see the below fictive example) and reduces the value of the other important documentation.

/// Moves the user to the provided channel
///
/// @arg{user} the user to move
/// @arg{channel} the channel to move the user into
bool moveUserToChannel(User user, Channel targetChannel) {
  // ...
}

It is great to document public API, but full coverage implies wasted time on meaningless documentation in my experience.

Other than that, I completetly agree with enforcing code guideline with tools and automated. Great idea!

[TerryGeng]

I like the idea of using the smart pointer! One notorious thing that keeps many developers away from C/C++ is manual memory management. Modernizing this part would make our life much easier :D.

And as for the coding guidelines, I believe this is absolutely necessary. During my primitive journey of contributing to mumble, I discovered that the codebase is in a variety of styles and sometimes it is hard to know what kind of style I should stick with. Fortunately, in my previous commits, @Krzmbrzl was willing to help me check my coding style and provide helpful instructions to make things neat. (Big thanks to him ^^)

However, the current diversity of coding styles of our codebase makes new contributors hard to follow a new style immediately, by simply inferring from old code. Therefore, to enforce a new style and a set of rules, we must make sure all old files are rewritten in accordance with the guidelines. It would be rather frustrating to be asked to fix the coding style when one is just trying to keep his code in line with the old code. I think we need to have a plan for this :).

However, personally I always welcome the mumble team to provide any kind of feedback on my PRs, and I'm always happy to make mumble neater and cleaner. And thank you guys again for developing such a good app and the support you have provided!

[Krzmbrzl]

Having had that enforced before, I would argue against it. It tends to mean documenting meaningless functions (see the below fictive example) and reduces the value of the other important documentation.

I see your point. However I think the approach of documenting "where appropriate" has several drawbacks as well (imo):

1. Not enforceable by an automatic check. How should my tool know which function needs documentation and which does not?
2. "Appropriate" is very subjective (same as "obvious"). For someone (especially the person that wrote the code) a function or variable name might be obvious, because that person knows what it does and what the context of that function is. If someone new comes to the code, they might not have that understanding though and for them documentation might be a big help.
   As an example take the audio processing part. For folks that are familiar with the topic, certain terms are just clear whereas someone with no strong background in that area might not know the terms and thus won't understand what certain code does.
3. I actually don't think that documentation for "obvious" functions is that much work after all 🤷

What I also thought about though, would be to lift the restrictions a bit in order to force documentation on all non-private functions. That way you could get away by not documenting some internal helper function in your class. I think this kind of rule could also still be verified automatically (although it probably takes quite a bit more effort to do so).

[vimpostor]

I mostly like the proposed guidelines, especially the smart pointer thing and clearly indicating what function takes ownership. However when a function does not take ownership, references should be preferred over raw pointers (if possible), as they guarantee to be not nullptr.

Are there any plans to move to a more modern C++ standard? Qt 6 is around the corner and will require C++17, so I would propose going the same direction.

[Krzmbrzl]

However when a function does not take ownership, references should be preferred over raw pointers (if possible), as they guarantee to be not nullptr.

Yeah absolutely. My statement about that was intended to be read as "if a pointer is required, use smart pointers if the function should take ownership and raw pointers otherwise". If however a reference can work as well, go with that 👍
I'll update the statement in my initial post ☝️

Are there any plans to move to a more modern C++ standard? Qt 6 is around the corner and will require C++17, so I would propose going the same direction.

Yes we will almost certainly use cpp17.

[Krzmbrzl]

@bendem @magnus-gross @Johni0702 do you have an idea how to overcome the problems of not enforcing documentation everywhere in order to allow a less strict rule for it?

[Krzmbrzl]

I have also been pointed to the following documents that contain c++ coding guidelines:

• https://www.autosar.org/fileadmin/user_upload/standards/adaptive/17-03/AUTOSAR_RS_CPP14Guidelines.pdf
• https://resources.sei.cmu.edu/downloads/secure-coding/assets/sei-cert-cpp-coding-standard-2016-v01.pdf

[Krzmbrzl]

Furthermore I just found https://github.com/cheshirekow/cmake_format

[Krzmbrzl]

In addition to clang-tidy we can use clazy that is designed to make clang understand Qt semnatics better and to include Qt-specific compiler warnings.