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

Replace NEST typedefs with plain C++ datatypes #2618

Merged
merged 31 commits into from
Jun 21, 2023
Merged

Conversation

pnbabu
Copy link
Contributor

@pnbabu pnbabu commented Feb 22, 2023

This PR fixes #2541. It replaces the following typedefs

  • index, thread, port, rport with size_t
  • weight with double
  • delay with long

@gtrensch gtrensch 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 Feb 24, 2023
@gtrensch gtrensch added this to PRs in progress in Kernel via automation Feb 24, 2023
Copy link
Contributor

@JanVogelsang JanVogelsang left a comment

Choose a reason for hiding this comment

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

These constant values here are fixed at compile time, thus using constexpr is a good thing to do here.
Apart from that, everything looks right to me.

nestkernel/nest_types.h Outdated Show resolved Hide resolved
nestkernel/nest_types.h Outdated Show resolved Hide resolved
nestkernel/nest_types.h Outdated Show resolved Hide resolved
nestkernel/nest_types.h Outdated Show resolved Hide resolved
nestkernel/nest_types.h Outdated Show resolved Hide resolved
nestkernel/nest_types.h Show resolved Hide resolved
@JanVogelsang
Copy link
Contributor

Edit: I didn't check yet if you missed any instances. Jochen, can you check that while you are reviewing?

models/ac_generator.cpp Outdated Show resolved Hide resolved
@heplesser
Copy link
Contributor

@pnbabu I have just merged #2616, which removes all the from-to asserts at the beginning of update() in models. This has unfortunately caused a lot of conflicts, but they should be straightforward to resolve by accepting the removal of the asserts.

@med-ayssar
Copy link
Contributor

@pnbabu any updates?

@heplesser
Copy link
Contributor

@pnbabu This looks good now overall, but having just read Stroustrup's "Tour", I am wondering about replacing so much with size_t (I know I argued for it before). The Core Guidelines don't say anything explicitly about size_t, but generally favour signed types (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-nonnegative) and point to suggest gsl::index for many cases where we currently use size_t, see https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines.html#Res-subscripts. This leaves me wondering a bit. @jougs what to you think?

@JanVogelsang
Copy link
Contributor

Looks good.
@heplesser Interesting, a few weeks ago I was asking myself the same question, as in university we got taught to use int for indexing, but personally I preferred size_t as well, because it seemed more descriptive. The core guidelines' arguments are valid, but I would have liked more depth, as I can't quite find anything regarding optimization/performance of unsigned vs signed types. I could only find an argument for unsigned types, as those can be divided by 2 faster than signed ones, but that's about it. And I am not sure why signed types enable better error detection either..
And apart from that, I don't really see an advantage of ptrdiff_t over size_t. In our code, we use size_t strictly for indexing or memory sizes, both of which can only be positive and will never be used in any operation that could return a negative value (or can you think of any?). And as we heavily rely on the STL, in my opinion, it makes more sense to stick with their "native" type.

Copy link
Contributor

@jougs jougs left a comment

Choose a reason for hiding this comment

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

Mostly fine by me. Just one repetitive pattern where I think the static_cast is not needed.

models/aeif_cond_beta_multisynapse.cpp Outdated Show resolved Hide resolved
models/correlomatrix_detector.cpp Outdated Show resolved Hide resolved
models/glif_psc.cpp Outdated Show resolved Hide resolved
models/aeif_cond_alpha_multisynapse.cpp Outdated Show resolved Hide resolved
models/iaf_psc_exp_multisynapse.cpp Outdated Show resolved Hide resolved
@jougs jougs merged commit 6d38161 into nest:master Jun 21, 2023
20 checks passed
Kernel automation moved this from PRs in progress to Done (PRs and issues) Jun 21, 2023
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.

Replace typedef'ed types with C++ PODs
6 participants