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

Modernize the codebase - overrides, nullptrs and redundant voids #2493

Merged
merged 4 commits into from
Oct 10, 2022

Conversation

JanVogelsang
Copy link
Contributor

@JanVogelsang JanVogelsang commented Oct 7, 2022

This PR acts as an addition to PR #2479. No new types of changes will be introduced by this PR, only missed cases of those discussed in the prequel-PR are added. These cover the following three types of issues:

  1. Using nullptr instead of NULL or 0 to initialize empty pointers or to check for an undefined pointer.
  2. Removing the redundant void argument from function with empty argument lists.
  3. Add missing override specifiers for overridden functions in derived classes and, if applicable, remove the redundant virtual specifier when override is set as well.

There are a few possible other changes that could be included in this PR:

  1. Mark overridden functions as final in models, as those will not be overridden any further. This is based on the assumption that models won't ever be based on another model (i.e., derive from another model). The final specifier is utilized by some compilers to increase performance of the compiled code. I have not seen any benchmarks yet that show how significant the performance increase can be, but even if performance is unaltered, marking a final function as final would hurt nobody.
  2. Change all occurrences of && to and as well as ! to not. We started doing this in Modernize the code base and make it more compliant to common C++ style guides #2479, and we should either do it everywhere or nowhere at all.
  3. Shorten if( ptr != nullptr ) to if ( ptr ) as both behave the same for all compilers that follow the C++ standard.

@heplesser
Copy link
Contributor

heplesser commented Oct 7, 2022

There are a few possible other changes that could be included in this PR:

I would advise against more complexity in this PR.

  1. Mark overridden functions as final in models, as those will not be overridden any further. This is based on the assumption that models won't ever be based on another model (i.e., derive from another model). The final specifier is utilized by some compilers to increase performance of the compiled code. I have not seen any benchmarks yet that show how significant the performance increase can be, but even if performance is unaltered, marking a final function as final would hurt nobody.

I tried final with SpikeEvent, because spike-delivery is significant for runtime and includes a large number of function calls (was a bit tricky since I had to de-inherit DSSpikeEvent). I did not see any change in runtime. Given the complexity of almost all methods in models, I would not hope for much. One could give it a little try with handle(SpikeEvent&), but I would not get my hope up high.

  1. Change all occurrences of && to and as well as ! to not. We started doing this in Modernize the code base and make it more compliant to common C++ style guides #2479, and we should either do it everywhere or nowhere at all.

Our policy so far has been to fix this if there are other changes to the code. I would suggest to isolate this in a separate PR if we want to do it everywhere.

  1. Shorten if( ptr != nullptr ) to if ( ptr ) as both behave the same for all compilers that follow the C++ standard.

Do it in this PR where you make changes involving nullptr anyways and then a separate PR.

@JanVogelsang
Copy link
Contributor Author

Sounds good, regarding no. 6, is there a good way of finding all occurrences of != nullptr in all lines that changed in a specific commit? Doing it by hand would be quite tedious..

@JanVogelsang
Copy link
Contributor Author

By the way, the MacOS check fails again without any errors, probably because of some warnings. These warnings are fixed in the other PR though, so I won't fix them here again. How should I handle this?

@heplesser
Copy link
Contributor

By the way, the MacOS check fails again without any errors, probably because of some warnings. These warnings are fixed in the other PR though, so I won't fix them here again. How should I handle this?

First get #2493 merged, then merge master into this PR, which should solve the problem. I would generally suggest that you build your work on the branch for #2493, so that when #2493 is merged, the change set from this PR simply becomes smaller.

@JanVogelsang
Copy link
Contributor Author

I did already do that, actually. All changes here are completely additional to those in #2479 . I now also merged master into this and will now wait to see if all warnings are gone.

@JanVogelsang JanVogelsang marked this pull request as ready for review October 7, 2022 15:54
@JanVogelsang
Copy link
Contributor Author

I addressed no. 6 in PR #2496 to keep different types of changes strictly separated. #2496 is based on the changes of this PR, we should therefore merge this PR first and then review the other one.
Both PRs should be fairly easy to review with less to discuss, as there were no special cases in both of them. The reviewing process should also be smoother now that I learned a lot from #2479.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks got to me, thank you! In the spirit of keeping PRs orthogonal, I think it is right to leave && and || in place here even in statements that are touched.

@heplesser heplesser requested a review from jougs October 7, 2022 21:00
@heplesser heplesser added S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation. I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Oct 7, 2022
@heplesser heplesser added this to PRs in progress in Kernel via automation Oct 7, 2022
@jougs jougs merged commit 0d4b7b1 into nest:master Oct 10, 2022
Kernel automation moved this from PRs in progress to Done (PRs and issues) Oct 10, 2022
@JanVogelsang JanVogelsang deleted the refactoring-sequel branch October 10, 2022 21:39
@jougs jougs changed the title Modernizing the codebase - overrides, nullptrs and redundant voids Modernize the codebase - overrides, nullptrs and redundant voids Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Kernel
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants