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

Mark overrideable functions as virtual #48

Closed
wants to merge 1 commit into from

Conversation

dargueta
Copy link

What

  • Mark all overrideable functions as virtual
  • Use the traditional virtual X = 0 to force overriding, instead of static asserts which are confusing to read (in my opinion).

Why

clang-tidy keeps yelling at me that my subclass is shadowing non-overrideable functions in the superclass. This also prevents us from using the override keyword to guard against typos and interface changes. By marking these functions as virtual

  • We explicitly specify which functions are expected to be overridden
  • We can use the override keyword to specify what we're overriding, and give the compiler enough information to catch mistakes in type signatures, naming (e.g. overriding a function that doesn't exist), and so on
  • We get rid of clang-tidy warnings/errors about shadowing.

@arpruss
Copy link
Collaborator

arpruss commented Mar 14, 2022

Does this have an impact on performance?

@dargueta
Copy link
Author

dargueta commented Mar 15, 2022

That is an excellent question. I'll try to do some performance testing and get back to you. Common wisdom says marking a function as virtual hurts performance, but from what I've read that's one of those "that used to be the case" *, like driving around after you get a jump will recharge your battery or drinking moonshine will make you go blind.

Only way to know for sure is to test.


* To be specific, cache misses loading the vtable. Modern CPUs have cache sizes well into the megabyte range.

@kosarev
Copy link
Owner

kosarev commented Mar 15, 2022

that's one of those "that used to be the case" *

Hi Diego, thanks for the patch. Relying on compile-time polymorphism is one of the most basic features of this implementation, enabling zero performance overhead. When necessary, it should be fairly trivial to employ virtual calls on top of the current approach whereas the opposite would be very much impossible. Regarding the clang-tidy warnings, would suppressing them be an option?

@arpruss
Copy link
Collaborator

arpruss commented Mar 15, 2022

Even if on the latest modern desktop and mobile systems there is no significant loss of performance, I can imagine use cases for this emulator on less powerful systems, like microcontrollers. My USB library for Arduino on stm32f1 initially used virtuals, but this resulted in significantly higher RAM usage (which mattered, because the microcontroller I was using has 20kb RAM and 128kb flash).

@dargueta
Copy link
Author

less powerful systems, like microcontrollers

Ah. I didn't think of that, sorry.

Regarding the clang-tidy warnings, would suppressing them be an option?

Presumably, if I can find my IDE's settings for it.

Anyway, I've been convinced this is a bad idea and will close it. Thanks for the feedback!

@dargueta dargueta closed this Mar 15, 2022
@dargueta dargueta deleted the virtual branch March 15, 2022 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants