Coding conventions discussion

[hendrikweisser]

Prompted by a discussion with @timosachsenberg, I checked the Coding Conventions page on our wiki: https://github.com/OpenMS/OpenMS/wiki/Coding-conventions

There are some conventions there that are incomplete or don't agree with current best practices. We may have reasons to stick with them anyway, but I believe it would be good to have a discussion and possibly make changes.

Examples:

• The code here says to always check for self-assignment. The C++ FAQ has a more nuanced view here - always handle self-assignment, but avoid the explicit check if possible (because self-assignments happen very rarely).
• For destructors (same section), the general recommendation is to make them virtual if a class has other virtual members: https://isocpp.org/wiki/faq/virtual-functions#virtual-dtors
• There is no mention in this section of when member functions can be automatically generated by the compiler - see here. Personally, I think we should avoid implementing functions that can be auto-generated, but the convention seems to suggest otherwise. No mention of using = default or = delete (C++11) either.

Please feel free to add other points for discussion as well.

[timosachsenberg]

we could just say: if possible follow the https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md :)

[hendrikweisser]

@timosachsenberg: We should include a link to them, but they are too long and in-depth to point a new contributor to and say: "Here, read this before writing code for OpenMS."

[hroest]

The code here says to always check for self-assignment. The C++ FAQ has a more nuanced view here - always handle self-assignment, but avoid the explicit check if possible (because self-assignments happen very rarely).

I dont have a strong view on that one

For destructors (same section), the general recommendation is to make them virtual if a class has other virtual members: https://isocpp.org/wiki/faq/virtual-functions#virtual-dtors

I agree, we should add that - I wonder which should be the default (virtual or not)

There is no mention in this section of when member functions can be automatically generated by the compiler - see here. Personally, I think we should avoid implementing functions that can be auto-generated, but the convention seems to suggest otherwise. No mention of using = default or = delete (C++11) either.

I think the coding conventions were written before C++11 so feel free to add that

[hroest]

We should probably switch to using = default when possibly and start to encourage the rule of five instead of the rule of three https://stackoverflow.com/questions/4782757/rule-of-three-becomes-rule-of-five-with-c11

So my intuition would be that the new default class should probably look like

class OPENMS_DLLAPI Test
{
  public:

    // default constructor
    Test() = default;
    Test(const Test&) = default;                            // Copy constructor
    Test(Test&&) noexcept = default;                        // Move constructor
    Test& operator=(const Test&) & = default;               // Copy assignment operator
    Test& operator=(Test&&) /*noexcept*/ & = default;       // Move assignment operator
    virtual ~Test();                                        // Destructor

private:
  String member1 = String();
  Int member2 = 0;
  double member3 = 0.0;
};

and this should probably replace default member initializers. Note that I have not added noexcept to the move assignment operator as this is not supported by gcc 4.8 (which we still need to support, see https://github.com/OpenMS/OpenMS/issues/980#issuecomment-198783912 )

We probably want to use & = default in all cases for compiler-generated and hand-written copy/move assignment operator (the ref qualifier) as this will prevent assignment to an rvalue, see [1] and hopefully prevent bugs such as

if ( func() = value )  // Typical typo: == intended!

1. https://stackoverflow.com/questions/24484365/what-does-in-c-operator-const-c-default-do

[hroest]

@hendrikweisser @timosachsenberg any thoughts on updating the coding style guide?

[timosachsenberg]

I agree. I already do this in newer classes.

[hendrikweisser]

any thoughts on updating the coding style guide?

Somebody should do it! ;-)

[hroest]

After the discussion here https://github.com/OpenMS/OpenMS/pull/3751#discussion_r219900729, the idea came up to use the "Rule of All or Nothing". That would mean that either all 5 are present or none are (then they are defaulted by the compiler). The question is how to check that, and I just read this nice article about how one could do that https://philippegroarke.com/posts/2018/easy_defensive_programming/ - basically then the default class would look like

class OPENMS_DLLAPI klass
{
public:
    // rule of Zero
private:
  String member1 = String();
  Int member2 = 0;
  double member3 = 0.0;
};

FULFILLS_RULE_OF_5(klass);

where the FULFILLS_RULE_OF_5(klass); would actually use static assertions to check that the rule of 5 is fulfilled. We could put FULFILLS_RULE_OF_5(klass); into the header (helps with documentation) or into the unit test file.

[hroest]

PS: we also have to deal with the question of the default move assignment operator that cannot be noexcept with gcc 4.8, see https://github.com/OpenMS/OpenMS/pull/3755#issuecomment-424075822

[hroest]

We should also have a policy on auto. Initially I thought it was a good thing to reduce clutter, but there are a few gotchas where it deduces an unexpected type so now I am not so sure any more. I think overall in OpenMS we have always used the guideline "explicit is better than implicit" and for auto that would mean to reduce its use unless there is a clear benefit and it really helps to improve readability.

See for example https://herbsutter.com/2018/09/20/lifetime-profile-v1-0-posted/ and example godbolt.org/z/iq5Qja for such a case.

[hroest]

Finally we could think about checking for const-ness in every move, as explained here http://yacoder.guru/blog/2015/05/06/cpp-curiosities-one-does-not-simply-move-a-const-object/ and use the suggested OpenMS::rvalue_cast instead of std::move

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.