Cxbx-reloaded: Define coding guidelines

[PatrickvL]

Instead of keeping around various issues on the preferred way of coding on cxbx, we should instead write up (and keep up to date) a coding guidelines document which stipulates and explains all those details

One if the issues that should be addressed, is "Constantify current codebase" https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/issues/13

[x1nixmzeng]

Bit off-topic but parts of the source could do with modernising - there is a mixed use of standard library (std::string) and C-style allocations.

So firstly - do we want to keep/enforce the Cxbx ASCII art? 😄

[LukeUsher]

I definitely want to modernise the source code, the C++ standard library is my preferred option.
Most of the issues stem with the Cxbx codebase being so old.

So let's start with the ASCII art, I think we can drop it because it doesn't serve a purpose, and increases the scroll distance from the top of the file to the main code.

Secondly, what about line-endings? I believe most are Windows style \r\n but this is not enforced, MSVC supports both that and unix style \n. Whatever is decided doesn't matter to me, so long as we have an agreed style. I'm leaning towards \r\n everywhere because this is a Windows application on a Windows development environment, but most projects I have ever worked on have been Unix style line endings.

Thirdly, for control statements and braces, I do have a preferred standard, all control structures should use braces, even if the following statement is a single line. This eliminates the risk of introducing errors by extending the code without adding correct braces, eg:

if (var == true) {
    var = false;
}

rather than

if (var == true)
    var = false;

Additionally, I prefer for braces to be on a new line for functions, but the same line for control statements, eg:

int main(int argc, char* argv[])
{
    for(int i = 1; i <= 100; i++) {
        if (i % 2 == 0) {
            printf("Even");
        } else {
            printf("Odd");
        }
    }

    return 0;
}

[RadWolfie]

I think ASCII art should be kept on main source file, all other source files should not have them. That's my opinion.

I remember there is a way to enforce line-endings with git as a standard for linux, mac, windows clients sending to git server. I had not done it before, might be worthy to do this way though. As in git server always serve line-endings as \r\n and clients can do what they like with their line-endings via git download and sync to server.

I do agree with @LukeUsher for both braces standard on function and single line process.

[PatrickvL]

What about empty lines between statement blocks? That visually keeps separate code blocks apart. It does however introduce yet another line, next to the ones containing just a closing curly brace. This might be troublesome when using a height-limited code viewer. Width-limited viewers are probably more interested in line-wrapping guidelines. Any ideas?

[PatrickvL]

We should also decide upon indent width, use of tabs or spaces, keeping or removing trailing whitespaces, newline style, declaration style of types, functions and macros, rules for symbol naming and caseing, alignment of adjoining lines with similar content, etc

[RadWolfie]

Excluding empty lines does not really bother me at all. If we do have a coder wishing to have this, then we could do that as well. My other projects doesn't use empty lines. For width-limited, I am totally not a big fan with it. It is a good thing I am just a hobbyist programmer, not a certified professional programmer. So I don't do it, unless Cxbx-Reloaded's guideline enforce it. I'll do my best to follow it.

It is my understanding tabs as indent is not recommend nor is a universal standard preference. Using spaces do keep the ident at "almost" exact same location with different fonts developer like to use in their editor. Plus, one tab is equal to 4 spaces as standard method.

[RadWolfie]

Here's a draft'd version of how to deal with new line ending and space requirement for .gitattributes file. Plus filtering will go in .gitconfig file as well. (see below) I found an instruction how to "refresh" the repo from github's article. With this method, it will enforce for us even if IDE we used are tab or whitespacing into the file. Plus it is cross-OS support for new line issue.

.gitattributes

# Standard new line ending
* text=auto
*.c text eol=lf
*.cpp text eol=lf
*.h text eol=lf
*.hpp text eol=lf
*.inl text eol=lf

EDIT: CRLF vs LF discussion had been determined to use LF for line ending.

---

OUTDATED, do not use below since it will not work.

# Standard space requirement
*.c filter=enforceTabSpace
*.cpp filter=enforceTabSpace
*.h filter=enforceTabSpace
*.hpp filter=enforceTabSpace
*.inl filter=enforceTabSpace

.gitconfig

 [filter "enforceTabSpace"]
     clean = expand --initial -t 4
     smudge = expand --initial -t 4
     required
 [merge]
     renormalize = true

[ergo720]

I'd like to add three more points:

1. there is a mix between include guards and the pragma once directive. I prefer the latter since it's faster to write and it has good compiler support (https://en.wikipedia.org/wiki/Pragma_once).
2. there is a mix between camel case and snake case in function/symbol naming, we should agree on which one to use.
3. I would remove the ASCII art since I saw that, when a file path is moved, very often the path is not updated as well, which results in incorrect paths. However, I would still maintain the GPL license and the names of the developers who contributed to the file

[PatrickvL]

Since we're not very actively chasing a well-defined coding guideline, nor are we doing much to enforce rules for it, I've created an issue for one of the things that has come up a few times already in the above comments : #1550 "Remove ASCII-art from header-comments in all our sources" (a beginners task).

Perhaps we can create simple sub-tasks for more of the remarks made above?

[RadWolfie]

I have update my outdated post of CRLF replace to LF for .gitattributes setup. And separate nonfunctional setup template since it will not work and recommended to use .clang-format handle tabs and ability to have action inspect pull requests for follow the coding standard.

[RadWolfie]

Our team have determine to use _xt, xbox type, and _ct, custom type, for any introduction or any discover of inconsistent types from current codebase in the repository.

Plus we all also agree using _tx, type xbox, and _tc, type custom does looks off.

An example of how we will implement _xt in our codebase: https://github.com/Cxbx-Reloaded/Cxbx-Reloaded/pull/1969