Question about coding style - use polymorphism/inheritance instead of #ifdefs?

ProffieOS
1

Hey all,

I’ve been poking around the proffie os source (ideally to get a teensy v4.1 board working), and had some questions.

First - some files use lots of #ifdef’s to handle the various hardware & options. This makes reading a single file difficult, since all the different code paths are intermingled.

What if we used virtual base classes & derived child classes instead?

For example: lsfs.h has a completely different implementation of LSFS for PROFFIE_TEST vs teensy vs (I think?) proffieboard, all in one file. This could be split into:

  • lsfs.h - holds the virtual lsfs base class/interface
  • lsfs_test.h - derived LSFSTest class for PROFFIE_TEST
  • lsfs_teensy.h - derived LSFSTeensy class
  • lsfs_proffie.h - derived LSFSProffie class

Pros

  • Much easier to focus on a single file, easier to focus/read what’s going on in a single implementation

Cons

  • More files to manage

And more generally, I’m thinking most classes could be arranged this way. Base classes implement the common/shared code, and anything device-specific goes into a derived implementation class.

An example of that could be the amplifier.h file. It handles both SGTL5000 as well as (I assume the proffie?). So the base class could have most everything, and the subclasses could implement only the Disable()/Enable() functions, etc.

Second - It seems like disabling some options gives compliation errors. Is there any kind of CI/CD in place yet? Would that be helpful to others if it were?

2

There is another con to consider:
Inheritance tends to have run-time overhead, and the compiler is generally not able to prune methods that are never used. It also limits inlining and other optimizations. I would not want to use inheritance for lsfs.h

Whether to split things out into separate files or not is generally an unrelated decision, and should be decided on a case-per-case basis. Generally, when the files gets to big, it’s time to split them up. I tend to put that limit slightly higher than some people, because navigating a lot of files is also painful in my opinion. lsfs.h is probably big enough that splitting it up would be helpful.

There is a test suite and it runs on github every time something is checked in. Unfortunately github does not have arduino files installed, so we’re limited to testing things that aren’t hardware dependent this way.

I also run “make test” every time I make a new zip file. “make test” tests a bunch of different config files and makes sure that they compile, and also runs the test suite. Obviously I can’t test every combination of options, but I do test a fair amount of them this way.

Ultimately, there are some combinations that just aren’t supported and will always generate errors, but I’m not sure if that’s what you’re seeing or not.

3

Inheritance tends to have run-time overhead

I figured that might be the case, makes sense.

lsfs.h is probably big enough that splitting it up would be helpful

I wouldn’t mind doing a PR, as long as that would be helpful and not more annoyance for the maintainers.

Is there a “So you want to help contribute” type guide, so I know I’m doing the right things here?

I also run “make test” every time I make a new zip file

Let me start here, make sure I’m seeing legit errors first.

4

Not really, but it’s not that complicated:

  • If you’re not sure if it’s a good idea, ask. (you already did this)
  • Keep PRs as short as possible
  • Be prepared for hard code reviews
1 Like