Coding style

[jarkkojs]

At the moment the code base is filled with a chaotic set of coding conventions. It would be good to have a well defined coding to which new commits would compared against. This would also make the patch review process more lean.

In order to move fast with this I would recommend to use some well defined style known to work in cross-platform projects e.g. https://webkit.org/code-style-guidelines. We could start for example with WebKit conventions and refine as we go.

Naturally, the existing code is how it is but this would be the convention for all future changes.

Other things to consider: the subset of C++ used (and related things like RTTI, most projects that aim for maximum portability do not use RTTI), external dependencies (are these allowed or not and in which circumstances) etc.

[jarkkojs]

Even things like, which type of newline character should be used, ought to be defined. This is actually the most high priority item because pull request are already flowing in. Usually with Git using LF is a better option than CRLF.

[asimilon]

Maybe a .clang-format file could be added to the repo? Visual Studio is supporting this AFAIK, and easy to install on macOS or Linux.

[esaruoho]

@asimilon would this help with the Apple Clang Error: Too many errors emitted, stopping now type stuff?

[asimilon]

No, just for auto formatting the source code to a chosen convention. It means anyone contributing doesn't really have to worry about coding style too much, as long as they run it through clang-format before committing.

[jarkkojs]

I prefer rather written guidelines. Don't even use clang for starters..

[asimilon]

You don't need to use clang in order to use clang-format. Who on earth is going to trawl the entire code base applying code style otherwise? I prefer computers do the boring stuff like that for me, and they don't make mistakes or forget once it's set up correctly. On the other hand humans quite often do. ;)

[jarkkojs]

It doesn't work like that in any open source project, at least not ones that I've actively worked. It is done patch per patch basis as part of the peer review process. You don't do commits just to change coding style.

Specific tool does not really replace a review process. Conventions can be also related to semantics like whether RTTI is allowed or not. Even with tooling you would have to first decide guidelines that you configure your tool to check. You cannot view this issue through some random utility.

[baconpaul]

This would be great to have.

The codebase is already getting pretty c++-11-y (and in some places c++-14-y) so I’d say “lets use C++14” and that means things like RTTI work but still have efficiency problems. But things like lambdas and std::function and so on work and can be used. (Since I used one to implement zoom I am particularly interested in this being true! :) )

A good setting that works for Xcode and emacs and meets your guidelines and an automatic formatting tool setting would both be wonderful. I’m pretty conscious of the fact that the PR’s I’ve been doing are a bit of a mish mash and it’s hard to match the code anyway.

Happy to follow any guideline the group suggests.

[jarkkojs]

Yeah, I've done only C and x86-assembly most of this decade (I do kernel development as my paid job) so my experiences are from pre-C++11-period :) As far as the standard level goes, yes, we need a roof and I'm with C++14.

I personally prefer to use vim without any automation and manual code review per patch. I'm fine with automatic formatting tool as long as it is not a replacement for manual code reviews, nor its use is enforced. We even do have one primitive in the kernel called checkpatch.pl that does basic checks.

I'm personally not too fond of of RTTI as it encourages easily write sloppy code. I think it is mainly a productivity tool. Not something that improves your software at run-time. You've just made bad design choices to require it. A base class with an enum and type() works suprisingly well and makes code easier to debug as things are more static at run-time. RTTI also decreases code portability or at least used to because of differences in implemtations. If we want to make Surge ever to compile as DLL (lets say the non-UI parts), this will increase complexity of achieving it.

Exception handling is another point of discussion. Whether to use return values or C++-exceptions. Again for maximum portability it is better not to use them but I don't have as strong standpoint here as with RTTI. I just hate the way the rape the resulting binary with all this crap for stack unwinding. When you have only the binary to debug they make it much nastier experience :) If we use exceptions, at minimum, lets define well defined rules how to declare and use them.

[jarkkojs]

I'll close this now with the conclusion that it does matter but is too early. Once "everything works" (heh) we could write some high-level conventions to the README.md. ATM run-time is broken except for a small subset of platform/target combinations and even the build/packaging is somewhat flakky. This is better topic to discuss later. I was way way too optimistic and ignorant when I wrote this.

I don't know if it is just me but only thing that really hurts my head is 3 spaces for tab :) In principle, there is nothing wrong with it but rest of the world uses 90% of projects either 4 character tabs with spaces or 8 character tabs with a tab character.