Standards or best practices or ....

Hi devs (and other interested people),

Running into an issue with one of my modules (see I had the most amazing jam last nigh, but this morning, Rack won't open. - #43 by Squinky) I realised this bug could have been easily avoided by some common practice (in this case checking for a return value of NULL). When I check other peoples code, I can see I am not the only one making this mistake, and actually I think I copied this bad practice from some other existing code. So this made me think, do we already have some Best practice guide for VCV development (Of course one can argue this should be common knowledge for C++ devs, but not all of us are C++ experts and we all make mistakes or forget stuff once in a while)? Or can we probably run our code through some VCV code correctness checker? The thinking is a bit in the direction of @baconpaul his LintBuddy ( A dev tool to look inside a module and check stuff). I am happy to pick this up, but of course only if people think it is helpful, valuable or not already available somewhere else. Let me know your thoughts.

4 Likes

Check that module isn’t null, step override calls the base class…

It’s funny. In my day job we are using a lot of typescript. This null json ptr thing would cause a compiler error in typescript…

2 Likes

Always a +1 vote for best practice descriptions and recipes from me. Don’t C++ compilers issue warnings for most potential correctness issues these days? Are there any good C++ linters? And yes, on top of that Rack correctness issues. A built-out Lintbuddy would come in very handy, although only so much can be checked at runtime, other things need source checking.

1 Like

I was thinking about a mix between runtime and source checking. The goal of this project should be a general plugin checker: Does it compile on all supported platforms, are all best practices implemented, is its runtime behaviour correct and according to best practices whatever they are … The result could be a value that scores the quality of the module (not from a sound perspective but coding).

Overall goal is to make sure the quality of the modules we deliver for test (and the library) improves.

5 Likes

Big :+1:

Perhaps these can be inspirations:

https://www.kernel.org/doc/html/v4.11/dev-tools/sparse.html

https://linux.die.net/man/1/ksc

1 Like

For your particular problem would it be solved by a higher level api which used std::optional on json gets? That would force a check or not compile. But requires 17.

I think an mit licensed set of c++17 rack helpers would be handy and if I was writing them the json wrangling is probably where I would start, along with a check for dirty in step and call bass class helper too. Those are patterns which seem a bit clunky.

I’d love to get less of these in 2.2.3.

It could help with the modules ones then standing out.

Various things I’ve seen during compile:

  • float x[CONST] = {}; bad lengths
  • signed/unsigned comparison
  • multiple strlen buffer length assumptions.
  • and a double indexed array index out of bounds warning, maybe single indexing by pointer?
1 Like

Hmm? Did you mean to respond to the top post?

Of course best practices have to emerge from somewhere. Out of curiosity, I decided to try to figure out when I started using the dataFromJson() override null pointer check before use “best practice”. As near as I can tell, I did so in 2020 when I added dataFromJson() override in Meander V1. As near as I can tell, I probably got this best practice from the Fundamental modules, but even at that time most (but not all) developers were using this same “practice”.

Of course - this forums reply semantics are way more perplexing than writing a module collection.

1 Like

Signed unsigned is almost never a problem in idiomatic rack; it comes usually from for loops where people use an int vs a size t or an auto without 0U

But if folks pursue this path let me encourage you to look at existing tools like the compiler warnings we have and clang tidy which already will do quite a lot.

So I got tired of having half finished safety measures across the three plugins I now have live so just made an sst-rackhelpers module which right now is only a submodule of airwin but will be a sub ofnsurge and bacon soon

I’ll move some stuff in there also and relicense it mit it including most of the surge modulation infra

For now I just gave the std optional json helpers

But it means you can check null and check type and get a default value all in one line

Requires c++17 but is handy

If anyone else wants to join in happy to take contributions and pull requests as long as we stay header only mit

Documentation and the like forthcoming but this was just me tired of copy and pasting the same boilerplate for a json read again and moving it and making it stricter

3 Likes

Yeah, a lot of warnings are “no big deal”. Esp for me since when I made plugins I used gcc as well as the Microsoft compiler, so between them there were a lot of warnings.

Still, zero warnings is a good policy. And for sure there are modules out there with super scary warnings.

1 Like

Yup. I toggle warnings to errors in mac in my CLion for surge and supress the ones I won’t fix explicitly.

I guess my point is between clang warnings, clang tidy, and modern c++ types, you can make the compiler solve a lot of the problems that used to be idiomatic reviews. But it requires some code, which is why I started collecting some of my idioms in a shareable spot.

1 Like

Is it really OK for library plugins to use c++ 17? I guess it must be, if you are.

Yes it is. The docker toolchain got updated to include it optionally around 2.0.6 or 2.1

That’s interesting. The plugin development manual still says C++ 11.

Yeah you have to do some makefile trickery but it all works

Basically you add the 17 flag then filter out the 11 or use the qno cmake stuff

Rack still builds 11 and 11 is the default but 17 works

2 Likes

I’m a big fan of Code Analysis tools to keep people on the same page. Easier in my work as we standardise on Visual Studio on one platform with the same language.