Update Contributor Guidelines

[n-eiling]

The contributor guidelines should be updated, and better enforced.
I think it's a good idea to just make people use clang-format and editorconf.
We should also document how commit messages should be formatted and where and how to comment things.

By enforce, I also mean, that we should not force people to use a style that is not documented in the guidelines. This leads to frustration and people not wanting to contribute.

[stv0g]

I fully agree. Do you maybe want to volunteer to draft an PR?

By enforce, I also mean, that we should not force people to use a style that is not documented in the guidelines. This leads to frustration and people not wanting to contribute.

But you still want to perform automatic checks in the CI that those rules are obeyed? Or not?

[n-eiling]

Let's collect here first, so I don't go into a wrong direction.
I would remove the current content of the page and refer to clang-format and editor conf for the basic styles.
Then we need to mention:

• file naming: Currently we use snake case
• naming of variables, files, classes: I would say, we prefer verbose names over short names, especially if they are outside facing. Avoid naming things similarly.
• commit messages: Currently we start with a capital letter, and refer to a module if this makes sense. We could switch to conventional commits? There is a linter that enforces conventional commits.
• comments: We want a comment in the file header to explain what the file does. Currently this is copied between header and .cpp
• SPDX: mention the author of every file. This is anyway enforced by the CI.
• Commented code that is not part of an explanation is not allowed.
• config file options: use snake case, add also to json schema.
• sign-off DCO. This is also enforced by the CI.

anything else you can think of?

When a PR is submitted that follows all the rules, a review should not require changes to new/undocumented rules. Instead we can add a commit to the PR changing this and also adding the new rule to the guidelines.

[stv0g]

Okay, I agree.

Overall, I would refer to pre-existing coding standards as much as possible.
E.g. the LLVM Coding Standards, since we already use the LLVM-preset in clang-format.

Here are my comments:

• File naming: Currently we use snake case
  • 👍🏻
  • also using .cpp and .hpp as C++ file extensions
• Naming of variables, files, classes:
  • I would say, we prefer verbose names over short names, especially if they are outside facing. Avoid naming things similarly.
    • 👍🏻
  • The LLVM Coding Standard says:

• Type names (including classes, structs, enums, typedefs, etc) should be nouns and start with an upper-case letter (e.g. TextFileReader).

• Variable names should be nouns (as they represent state). The name should be camel case, and start with an upper case letter (e.g. Leader or Boats).

• Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

• Enum declarations (e.g. enum Foo {...}) are types, so they should follow the naming conventions for types. A common use for enums is as a discriminator for a union, or an indicator of a subclass. When an enum is used for something like this, it should have a Kind suffix (e.g. ValueKind).

• Enumerators (e.g. enum { Foo, Bar }) and public member variables should start with an upper-case letter, just like types. Unless the enumerators are defined in their own small namespace or inside a class, enumerators should have a prefix corresponding to the enum declaration name. For example, enum ValueKind { ... }; may contain enumerators like VK_Argument, VK_BasicBlock, etc. Enumerators that are just convenience constants are exempt from the requirement for a prefix.

• Commit messages:
  • Currently we start with a capital letter, and refer to a module if this makes sense. We could switch to conventional commits? There is a linter that enforces conventional commits.
  • +1 for conventional commits here. This would also allow us to automatically tag new versions by the CI following the semantic versioning rule
• Comments:
  • We want a comment in the file header to explain what the file does. Currently this is copied between header and .cpp
  • Can we agree on having this only in the header file to avoid duplication and inconsistencies.
  • See also: https://llvm.org/docs/CodingStandards.html#commenting
• SPDX: mention the author of every file.
  • This is anyway enforced by the CI.
• Commented code that is not part of an explanation is not allowed.
  • 👍🏻
• Config file options: use snake case, add also to json schema.
  • 👍🏻
• Sign-off DCO. This is also enforced by the CI.
  • 👍🏻

[n-eiling]

Regarding naming of variables: Do you know why they capitalize variable names? To me it seems like this would remove the differentiation with class names.
As far as I know, our current code base always uses lower case.

While I see the benefit of adopting something standard, looking over the LLVM guide it seems to me very strict. My intention of updating our contributor guidelines was to make it easier to know what will be accepted. But the LLVM guidelines seem more strict that what we enforce.

I feel like, as long as it stay relatively short, it would be better if we just copy the most important guidelines from the LLVM guidelines and not blankedly say, we adhere to all of them.

[n-eiling]

To be discussed:

• Exceptions (I would discourage the use)
• RTTI (I would discourage the use, although this is quite entrenched in the code base)

[n-eiling]

@stv0g and me discussed exceptions offline and decided to not allow exceptions in new code and, over time. remove the existing exceptions.

My suggestion for error handling would be to use std::expected as return values. From my understanding it works similar to optionals in other languages. A calling function should then always check for errors explicitly before using a return value.
It seems to also support the chaining you were talking about with the or_else() call : see here

I would probably implement a typedef for std::expected with just a string for the error type so that we can change this if there is a need.

This is a C++23 feature but there are supposed to be implementations for earlier version.

Another thing I found was std::basic_stacktrace which we could optionally add to the error type. Perhaps we can create the stack trace in the constructor of the a std::expected so we always get a stack trace for free. Maybe this needs a compile time flag to deactivate for performance.

Regarding RTTI, I would suggest saying it is discourages in new code.

@stv0g what do you think?