Removed _t aliases for built-in C++ data types, see #343. #454

Merged
merged 5 commits into from Aug 26, 2016

Conversation

Projects
None yet
4 participants
@heplesser
Contributor

heplesser commented Aug 8, 2016

This PR removes all _t "same name" aliases of C++ PODs (double_t, float_t, int_t, long_t, uint_t, ulong_t) from NEST, fixing #343.

I suggest @apeyser and @terhorstd as reviewers.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Aug 9, 2016

Contributor

Found these, mostly in comments from the looks of it:

[asinha@cs-as14aho-2-herts-ac-uk  nest-simulator(fix-343-remove-pod-alias-types=)]$ grep -Erni " (double_t|float_t|int_t|long_t|uint_t|ulong_t)" *

models/hh_psc_alpha_gap.h:190:   * @param double_t new value of the mebrane potential
models/pp_psc_delta.cpp:329:  // a double_t in ms. The grid based iaf_psp_delta can only handle refractory
models/binary_neuron.h:92:  // must have an double_t operator(double_t) defined
models/mat2_psc_exp.cpp:279:  // a double_t in ms. The grid based mat2_psc_exp can only handle refractory
models/iaf_psc_exp_multisynapse.h:206:    //    double_t PSCInitialValue_;
models/amat2_psc_exp.h:310:    //    double_t PSCInitialValue_;
models/music_message_in_proxy.cpp:161:    double_t acceptable_latency = P_.acceptable_latency_ / 1000.0;
models/iaf_tum_2000.h:278:    //    double_t PSCInitialValue_;
models/iaf_tum_2000.cpp:259:  // a double_t in ms. The grid based iaf_tum_2000 can only handle refractory
models/iaf_psc_alpha.cpp:265:  // a double_t in ms. The grid based iaf_psc_alpha can only handle refractory
models/iaf_neuron.cpp:234:  // a double_t in ms. The grid based iaf_neuron can only handle refractory
models/amat2_psc_exp.cpp:351:  // a double_t in ms. The grid based amat2_psc_exp can only handle refractory
models/mat2_psc_exp.h:282:    //    double_t PSCInitialValue_;
models/iaf_chs_2007.h:233:    //    double_t PSCInitialValue_;
models/music_event_out_proxy.cpp:222:  double_t time = e.get_stamp().get_ms() * 1e-3; // event time in seconds
models/hh_psc_alpha.h:167:   * @param double_t new value of the mebrane potential
models/iaf_psc_exp.cpp:257:  // a double_t in ms. The grid based iaf_psc_exp can only handle refractory
models/iaf_psc_delta.cpp:234:  // a double_t in ms. The grid based iaf_psp_delta can only handle refractory
models/iaf_psc_exp.h:280:    //    double_t PSCInitialValue_;
nestkernel/ring_buffer.h:89:   * @param  double_t Value to add.
nestkernel/ring_buffer.h:96:   * @param  double_t Value to set.
nestkernel/ring_buffer.h:206:   * @param  double_t Value to add.
nestkernel/ring_buffer.h:287:   * @param  double_t Value to append.
nestkernel/connector_model_impl.h:98:// double_t get_default_delay(const GenericConnectorModel<ConnectionT> &cm)
nestkernel/archiving_node.h:75:   * \fn double_t get_Ca_minus()
nestkernel/archiving_node.h:81:   * \fn double_t get_synaptic_elements(Name n)
nestkernel/archiving_node.h:104:   * \fn std::map<Name, double_t> get_synaptic_elements()
nestkernel/archiving_node.h:130:   * \fn double_t get_K_value(long t)
nestkernel/archiving_node.h:144:   * \fn double_t get_triplet_K_value(std::deque<histentry>::iterator &iter)
nestkernel/archiving_node.h:181:   * \fn void set_spiketime(Time const & t_sp, double_t offset)
nestkernel/archiving_node.h:187:   * \fn double_t get_spiketime()
nestkernel/mpi_manager.h:265:   * @note This class actually stores the GID as @c double_t internally.
nestkernel/node.h:590:   * @param t double_t time when the update is being performed
nestkernel/nest_time.h:148:  // double_t: milliseconds (double!)
nestkernel/nest_time.h:231:#define LIM_POS_INF_ms double_t_max // because C++ bites
nestkernel/music_manager.h:59:void set_music_in_port_acceptable_latency( std::string portname, double_t
nestkernel/synaptic_element.h:167:  * \fn double_t get_z_value(Archiving_Node const *a, double_t t) const
nestkernel/music_event_handler.cpp:98:    double_t acceptable_latency = acceptable_latency_ / 1000.0;
nestkernel/nest_types.h:101:const long long_t_max = LONG_MAX;
nestkernel/nest_types.h:102:const long long_t_min = LONG_MIN;
nestkernel/nest_types.h:110:#define double_t_max ( DBL_MAX )
nestkernel/nest_types.h:111:#define double_t_min ( DBL_MIN )
nestkernel/nest_types.h:190:const long delay_max = long_t_max;
nestkernel/nest_types.h:191:const long delay_min = long_t_min;
precise/iaf_psc_exp_ps.h:241:   * @param   double_t length of interval since previous event
precise/iaf_psc_alpha_canon.h:274:   * @param   double_t length of interval since previous event
sli/sli_io.cc:1320:    Token int_token( new IntegerDatum( c ) );
sli/sli_io.cc:1322:    i->OStack.push_move( int_token );
sli/fdstream.h:122:  int_type
sli/fdstream.h:135:  int_type
sli/fdstream.h:136:  overflow( int_type c )
topology/doc/Topology_UserManual.tex:1405:    double_t rx_, ry_;
Contributor

sanjayankur31 commented Aug 9, 2016

Found these, mostly in comments from the looks of it:

[asinha@cs-as14aho-2-herts-ac-uk  nest-simulator(fix-343-remove-pod-alias-types=)]$ grep -Erni " (double_t|float_t|int_t|long_t|uint_t|ulong_t)" *

models/hh_psc_alpha_gap.h:190:   * @param double_t new value of the mebrane potential
models/pp_psc_delta.cpp:329:  // a double_t in ms. The grid based iaf_psp_delta can only handle refractory
models/binary_neuron.h:92:  // must have an double_t operator(double_t) defined
models/mat2_psc_exp.cpp:279:  // a double_t in ms. The grid based mat2_psc_exp can only handle refractory
models/iaf_psc_exp_multisynapse.h:206:    //    double_t PSCInitialValue_;
models/amat2_psc_exp.h:310:    //    double_t PSCInitialValue_;
models/music_message_in_proxy.cpp:161:    double_t acceptable_latency = P_.acceptable_latency_ / 1000.0;
models/iaf_tum_2000.h:278:    //    double_t PSCInitialValue_;
models/iaf_tum_2000.cpp:259:  // a double_t in ms. The grid based iaf_tum_2000 can only handle refractory
models/iaf_psc_alpha.cpp:265:  // a double_t in ms. The grid based iaf_psc_alpha can only handle refractory
models/iaf_neuron.cpp:234:  // a double_t in ms. The grid based iaf_neuron can only handle refractory
models/amat2_psc_exp.cpp:351:  // a double_t in ms. The grid based amat2_psc_exp can only handle refractory
models/mat2_psc_exp.h:282:    //    double_t PSCInitialValue_;
models/iaf_chs_2007.h:233:    //    double_t PSCInitialValue_;
models/music_event_out_proxy.cpp:222:  double_t time = e.get_stamp().get_ms() * 1e-3; // event time in seconds
models/hh_psc_alpha.h:167:   * @param double_t new value of the mebrane potential
models/iaf_psc_exp.cpp:257:  // a double_t in ms. The grid based iaf_psc_exp can only handle refractory
models/iaf_psc_delta.cpp:234:  // a double_t in ms. The grid based iaf_psp_delta can only handle refractory
models/iaf_psc_exp.h:280:    //    double_t PSCInitialValue_;
nestkernel/ring_buffer.h:89:   * @param  double_t Value to add.
nestkernel/ring_buffer.h:96:   * @param  double_t Value to set.
nestkernel/ring_buffer.h:206:   * @param  double_t Value to add.
nestkernel/ring_buffer.h:287:   * @param  double_t Value to append.
nestkernel/connector_model_impl.h:98:// double_t get_default_delay(const GenericConnectorModel<ConnectionT> &cm)
nestkernel/archiving_node.h:75:   * \fn double_t get_Ca_minus()
nestkernel/archiving_node.h:81:   * \fn double_t get_synaptic_elements(Name n)
nestkernel/archiving_node.h:104:   * \fn std::map<Name, double_t> get_synaptic_elements()
nestkernel/archiving_node.h:130:   * \fn double_t get_K_value(long t)
nestkernel/archiving_node.h:144:   * \fn double_t get_triplet_K_value(std::deque<histentry>::iterator &iter)
nestkernel/archiving_node.h:181:   * \fn void set_spiketime(Time const & t_sp, double_t offset)
nestkernel/archiving_node.h:187:   * \fn double_t get_spiketime()
nestkernel/mpi_manager.h:265:   * @note This class actually stores the GID as @c double_t internally.
nestkernel/node.h:590:   * @param t double_t time when the update is being performed
nestkernel/nest_time.h:148:  // double_t: milliseconds (double!)
nestkernel/nest_time.h:231:#define LIM_POS_INF_ms double_t_max // because C++ bites
nestkernel/music_manager.h:59:void set_music_in_port_acceptable_latency( std::string portname, double_t
nestkernel/synaptic_element.h:167:  * \fn double_t get_z_value(Archiving_Node const *a, double_t t) const
nestkernel/music_event_handler.cpp:98:    double_t acceptable_latency = acceptable_latency_ / 1000.0;
nestkernel/nest_types.h:101:const long long_t_max = LONG_MAX;
nestkernel/nest_types.h:102:const long long_t_min = LONG_MIN;
nestkernel/nest_types.h:110:#define double_t_max ( DBL_MAX )
nestkernel/nest_types.h:111:#define double_t_min ( DBL_MIN )
nestkernel/nest_types.h:190:const long delay_max = long_t_max;
nestkernel/nest_types.h:191:const long delay_min = long_t_min;
precise/iaf_psc_exp_ps.h:241:   * @param   double_t length of interval since previous event
precise/iaf_psc_alpha_canon.h:274:   * @param   double_t length of interval since previous event
sli/sli_io.cc:1320:    Token int_token( new IntegerDatum( c ) );
sli/sli_io.cc:1322:    i->OStack.push_move( int_token );
sli/fdstream.h:122:  int_type
sli/fdstream.h:135:  int_type
sli/fdstream.h:136:  overflow( int_type c )
topology/doc/Topology_UserManual.tex:1405:    double_t rx_, ry_;
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 10, 2016

Contributor

@sanjayankur31 Thanks for noticing those leftovers. They should now be gone.

Contributor

heplesser commented Aug 10, 2016

@sanjayankur31 Thanks for noticing those leftovers. They should now be gone.

@sanjayankur31

This comment has been minimized.

Show comment
Hide comment
@sanjayankur31

sanjayankur31 Aug 10, 2016

Contributor

Re-ran a tweaked grep. Are we not removing longlong_t?

sli/slistartup.cc:557:  typedef long long longlong_t;
sli/slistartup.cc:560:  typedef long longlong_t;
sli/slistartup.cc:566:    longlongsize_name, Token( new IntegerDatum( sizeof( longlong_t ) ) ) );

Some limits are still using the _t convention - are these to stay?

nestkernel/nest_time.h:231:#define LIM_POS_INF_ms double_t_max // because C++ bites
nestkernel/nest_time.h:238:#define LIM_NEG_INF_ms ( -double_t_max ) // c++ bites
nestkernel/simulation_manager.h:158:  bool print_time_; //!< Indicates whether time should be printed during
nestkernel/kernel_manager.h:93: print_time               booltype    - Whether to print progress information during the simulation
nestkernel/nest_types.h:101:const long long_t_max = LONG_MAX;
nestkernel/nest_types.h:102:const long long_t_min = LONG_MIN;
nestkernel/nest_types.h:110:#define double_t_max ( DBL_MAX )
nestkernel/nest_types.h:111:#define double_t_min ( DBL_MIN )
nestkernel/nest_types.h:190:const long delay_max = long_t_max;
nestkernel/nest_types.h:191:const long delay_min = long_t_min;

Otherwise LGTM - the test is failing because fglrx couldn't be loaded - not really sure what's happening there.

Contributor

sanjayankur31 commented Aug 10, 2016

Re-ran a tweaked grep. Are we not removing longlong_t?

sli/slistartup.cc:557:  typedef long long longlong_t;
sli/slistartup.cc:560:  typedef long longlong_t;
sli/slistartup.cc:566:    longlongsize_name, Token( new IntegerDatum( sizeof( longlong_t ) ) ) );

Some limits are still using the _t convention - are these to stay?

nestkernel/nest_time.h:231:#define LIM_POS_INF_ms double_t_max // because C++ bites
nestkernel/nest_time.h:238:#define LIM_NEG_INF_ms ( -double_t_max ) // c++ bites
nestkernel/simulation_manager.h:158:  bool print_time_; //!< Indicates whether time should be printed during
nestkernel/kernel_manager.h:93: print_time               booltype    - Whether to print progress information during the simulation
nestkernel/nest_types.h:101:const long long_t_max = LONG_MAX;
nestkernel/nest_types.h:102:const long long_t_min = LONG_MIN;
nestkernel/nest_types.h:110:#define double_t_max ( DBL_MAX )
nestkernel/nest_types.h:111:#define double_t_min ( DBL_MIN )
nestkernel/nest_types.h:190:const long delay_max = long_t_max;
nestkernel/nest_types.h:191:const long delay_min = long_t_min;

Otherwise LGTM - the test is failing because fglrx couldn't be loaded - not really sure what's happening there.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 10, 2016

Contributor

@sanjayankur31 LGTM??? Travis has some technical problems according to the status info on their site that probably causes the failures.

Concerning longlong_t, I had overlooked it because it is defined in the code for the SLI interpreter, not the NEST kernel. But it is used solely in creating the longlong have/size entries in the statusdict/architecture dictionary, so I will remove it.

@apeyser Concerning the {double,long}_t_{min, max} defined in nest_types.h: Do we really need those defines? Why don't we use the {DOUBLE,LONG}_{MIN,MAX} from the standard headers directly?

Contributor

heplesser commented Aug 10, 2016

@sanjayankur31 LGTM??? Travis has some technical problems according to the status info on their site that probably causes the failures.

Concerning longlong_t, I had overlooked it because it is defined in the code for the SLI interpreter, not the NEST kernel. But it is used solely in creating the longlong have/size entries in the statusdict/architecture dictionary, so I will remove it.

@apeyser Concerning the {double,long}_t_{min, max} defined in nest_types.h: Do we really need those defines? Why don't we use the {DOUBLE,LONG}_{MIN,MAX} from the standard headers directly?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 10, 2016

Contributor

See also #439 for failing builds due to fglrx not loading.

@heplesser: LGTM = looks good to me. See also http://lgtm.in/

Contributor

jougs commented Aug 10, 2016

See also #439 for failing builds due to fglrx not loading.

@heplesser: LGTM = looks good to me. See also http://lgtm.in/

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 10, 2016

Contributor

@heplesser: I'm not sure if the issue reported on the status page of Travis CI are causing our problems. I've emailed their support for further assistance.

Contributor

jougs commented Aug 10, 2016

@heplesser: I'm not sure if the issue reported on the status page of Travis CI are causing our problems. I've emailed their support for further assistance.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 10, 2016

Contributor

Our failing builds actually seem to be a Travis bug. See travis-ci/packer-templates#221 for the issue they opened after my mail.

Contributor

jougs commented Aug 10, 2016

Our failing builds actually seem to be a Travis bug. See travis-ci/packer-templates#221 for the issue they opened after my mail.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Aug 11, 2016

Contributor

@heplesser Why do we have those variables? Because steps were long_t, and I had to search for what a long_t was and what it's max should be, so I labelled it. Since it was an alias that could be changed (I assumed, since why else alias), I wanted the label generalized in case that actually happened. (Further comment for historical record, steps were int_t with a complicated system of range checks that were simplified by going to long_t).

But if we get rid of long_t, there's no reason to label the max. The other option was to assume that long_t was a primitive type and use a numeric_limits type, but that wouldn't work for const types before constexpr in C++11. #453

Thus, we should remove them now and use {DOUBLE,LONG}_{MIN,MAX}.

Contributor

apeyser commented Aug 11, 2016

@heplesser Why do we have those variables? Because steps were long_t, and I had to search for what a long_t was and what it's max should be, so I labelled it. Since it was an alias that could be changed (I assumed, since why else alias), I wanted the label generalized in case that actually happened. (Further comment for historical record, steps were int_t with a complicated system of range checks that were simplified by going to long_t).

But if we get rid of long_t, there's no reason to label the max. The other option was to assume that long_t was a primitive type and use a numeric_limits type, but that wouldn't work for const types before constexpr in C++11. #453

Thus, we should remove them now and use {DOUBLE,LONG}_{MIN,MAX}.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 11, 2016

Contributor

@apeyser Thanks for the clarification, I will update the PR shortly. Is there a particular reason why we are using DBL_MAX etc instead of std::numeric_limits<double>::max() and the like? I have a hunch that it has something to do with the "because C++ bites" comments?

Contributor

heplesser commented Aug 11, 2016

@apeyser Thanks for the clarification, I will update the PR shortly. Is there a particular reason why we are using DBL_MAX etc instead of std::numeric_limits<double>::max() and the like? I have a hunch that it has something to do with the "because C++ bites" comments?

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Aug 11, 2016

Contributor

@heplesser Yes --- exactly. In C++ < 11, a floating point constant can only be initialized with a floating point literal. Thus, #define for the double_t_max. For integer types, you can defined a constant in terms of another constant under very limited conditions (literal and const arithmetic expressions, basically). So std::numeric_limits::max() can not be used to initialize a const, which means that it's difficult for the compiler to deduce the const-ness of the value throughout the program, which made many traits not-very-useful.

Thus, 15 years on after C++ being first widely used, const weren't const's. But C++11 adds constexpr that come much closer to allowing you to use as a const terms that are composed in ways that are easily deducible to be consts.

Contributor

apeyser commented Aug 11, 2016

@heplesser Yes --- exactly. In C++ < 11, a floating point constant can only be initialized with a floating point literal. Thus, #define for the double_t_max. For integer types, you can defined a constant in terms of another constant under very limited conditions (literal and const arithmetic expressions, basically). So std::numeric_limits::max() can not be used to initialize a const, which means that it's difficult for the compiler to deduce the const-ness of the value throughout the program, which made many traits not-very-useful.

Thus, 15 years on after C++ being first widely used, const weren't const's. But C++11 adds constexpr that come much closer to allowing you to use as a const terms that are composed in ways that are easily deducible to be consts.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Aug 25, 2016

Contributor

@heplesser looks like you've been stepped on by another merge...

Contributor

apeyser commented Aug 25, 2016

@heplesser looks like you've been stepped on by another merge...

Merge branch 'master' of git://github.com/nest/nest-simulator into fi…
…x-343-remove-pod-alias-types

# Conflicts:
#	nestkernel/event_delivery_manager.h
@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 25, 2016

Contributor

@apeyser I have now merged all new commits from master and the conflicts are resolved.

Once we merge this PR, we need to inform all users via the mailing list that they need to pull these changes into their PRs and adapt their code.

Contributor

heplesser commented Aug 25, 2016

@apeyser I have now merged all new commits from master and the conflicts are resolved.

Once we merge this PR, we need to inform all users via the mailing list that they need to pull these changes into their PRs and adapt their code.

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Aug 25, 2016

Contributor

👍 Will merge if/when checks succeed

Contributor

apeyser commented Aug 25, 2016

👍 Will merge if/when checks succeed

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Aug 25, 2016

Contributor

Should we add a new regression test that greps for the unwanted types? This way users are directly made aware of problems that may arise.

Contributor

jougs commented Aug 25, 2016

Should we add a new regression test that greps for the unwanted types? This way users are directly made aware of problems that may arise.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 25, 2016

Contributor

@jougs Doesn't the compiler provide that regression test with much better quality (no false positives) than any grep could? A script would only protect us against a developer re-inserting a typedef for double_t & Co. Or am I overlooking something?

Contributor

heplesser commented Aug 25, 2016

@jougs Doesn't the compiler provide that regression test with much better quality (no false positives) than any grep could? A script would only protect us against a developer re-inserting a typedef for double_t & Co. Or am I overlooking something?

@apeyser apeyser merged commit 0d58acc into nest:master Aug 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@heplesser heplesser deleted the heplesser:fix-343-remove-pod-alias-types branch Aug 26, 2016

@Silmathoron Silmathoron referenced this pull request Sep 6, 2016

Merged

Gif models #261

janhahne added a commit to janhahne/nest-simulator that referenced this pull request May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment