Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make MFPRenderer inherit Renderer instead of using composition #13

Open
josephlarralde opened this issue Jul 19, 2022 · 1 comment · May be fixed by #21
Open

Make MFPRenderer inherit Renderer instead of using composition #13

josephlarralde opened this issue Jul 19, 2022 · 1 comment · May be fixed by #21
Labels
enhancement New feature or request

Comments

@josephlarralde
Copy link
Contributor

This would be much more elegant and less error-prone if we didn't have to write T1 function(T2 arg) { return renderer.function(arg); } in MFPRenderer for each method of Renderer we want to expose.

After a bunch of failed attempts, it seems that it is not possible to achieve this using inheritance in C++.

I tried :

template <typename Model, typename Command, typename CommandKey>
class MFPRenderer : public Renderer<Model, Command, CommandKey> { /* ... */ }

and use a specialized version MFPRenderer<noteData, commandData, commandKey> with only explicit specializations of methods which required modifications.

I also tried :

class MFPRenderer : public Renderer<noteData, commandData, commandKey> { /* ... */ }

In both cases, we inherit from a class template we never fully explicitly specialize or instantiate, so all the functions that are not specialized with the concrete types are undefined, and although compilation succeeds with emscripten we end up with ... is not a function errors in the JS code.

I tried to create dummy instances of Renderer<noteData, commandData, commandKey> in the code (as static and local MFPRenderer variables), but this didn't help, maybe because of some compiler optimizations, or maybe because I don't grasp the issue very well ...

The only way to get inheritance to work (in both cases), is to write stuff like this for every public member

T1 function(T2 arg) { return Renderer<Model, Command, CommandKey>::function(arg); }

in the first case and

T1 function(T2 arg) { return Renderer<noteData, commandData, commandKey>::function(arg); }

in the second case which is as verbose and error-prone as the composition based design.

So, if anyone comes up with a better solution, I'm all ears.
Meanwhile, I'll stick with the composition based design.

@josephlarralde josephlarralde added the enhancement New feature or request label Jul 19, 2022
@josephlarralde
Copy link
Contributor Author

PR #21 proposes an alternative lib architecture where we have simple Chronology and Renderer classes, and a new Performer class has a Chronology and a Renderer (or several of them, depending on the implementation).
This is more flexible and reduces code duplication.

@josephlarralde josephlarralde linked a pull request Aug 30, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant