Replace operator<< with regular function to avoid issues with K compiler #587

Merged
merged 1 commit into from Dec 19, 2016

Conversation

Projects
None yet
5 participants
@jakobj
Contributor

jakobj commented Dec 11, 2016

This PR fixes an incompatibility with the K compiler as discussed in #581.
I suggest @janhahne and @JanneM as reviewers.

nestkernel/connection_manager.cpp
-static inline std::deque< nest::ConnectionID >& operator<<(
- std::deque< nest::ConnectionID >& out,
+static inline std::deque< nest::ConnectionID >&
+join_deques( std::deque< nest::ConnectionID >& out,

This comment has been minimized.

@heplesser

heplesser Dec 11, 2016

Contributor

The static is confusing here, since this use of static is so rare hardly anyone knows what it does. It does make sense, as it makes sure there won't be any conflicts with functions of same name and signature defined in other cpp-files, but you should probably comment on this here.

I am also wondering a little about the name. Maybe extend( connectome, conns_in_thread ) or add_to( connectome, conns_in_thread ) would make for even more readable code.

@heplesser

heplesser Dec 11, 2016

Contributor

The static is confusing here, since this use of static is so rare hardly anyone knows what it does. It does make sense, as it makes sure there won't be any conflicts with functions of same name and signature defined in other cpp-files, but you should probably comment on this here.

I am also wondering a little about the name. Maybe extend( connectome, conns_in_thread ) or add_to( connectome, conns_in_thread ) would make for even more readable code.

This comment has been minimized.

@JanneM

JanneM Dec 12, 2016

Contributor

I don't have enough experience with the code base to review the patch, I think. But I agree that the name should reflect what the code does to nest rather than what it does to the data structure.

The original code uses static, so it makes sense to keep it for this patch. If it's unneeded, then it's probably better to create a separate issue to change that.

@JanneM

JanneM Dec 12, 2016

Contributor

I don't have enough experience with the code base to review the patch, I think. But I agree that the name should reflect what the code does to nest rather than what it does to the data structure.

The original code uses static, so it makes sense to keep it for this patch. If it's unneeded, then it's probably better to create a separate issue to change that.

This comment has been minimized.

@jakobj

jakobj Dec 13, 2016

Contributor

I agree with Janne on the second part. Regarding the name I don't have a strong opinion, I would be fine with extend or even something more specific extend_connectome that actually describes would be would like to achieve here.

@jakobj

jakobj Dec 13, 2016

Contributor

I agree with Janne on the second part. Regarding the name I don't have a strong opinion, I would be fine with extend or even something more specific extend_connectome that actually describes would be would like to achieve here.

This comment has been minimized.

@heplesser

heplesser Dec 13, 2016

Contributor

extend_connectome() should be fine.

@heplesser

heplesser Dec 13, 2016

Contributor

extend_connectome() should be fine.

@terhorstd terhorstd added S: Critical and removed S: High labels Dec 12, 2016

@janhahne

janhahne approved these changes Dec 13, 2016 edited

Nice fix! I assume that you compiled it on K and can confirm that it works there (because I didn't)?

Once there is an agreement on the name of the function, I am 👍 for merging

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Dec 13, 2016

Contributor

Thanks @janhahne. Yes I compiled it on K (it works) and will do so again, before merging, after we have settled on a name.

Contributor

jakobj commented Dec 13, 2016

Thanks @janhahne. Yes I compiled it on K (it works) and will do so again, before merging, after we have settled on a name.

@jakobj

This comment has been minimized.

Show comment
Hide comment
@jakobj

jakobj Dec 19, 2016

Contributor

Renamed and made sure that it compiles on K. A second thumbs up anyone?

Contributor

jakobj commented Dec 19, 2016

Renamed and made sure that it compiles on K. A second thumbs up anyone?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Dec 19, 2016

Contributor

👍 and merging.

Contributor

heplesser commented Dec 19, 2016

👍 and merging.

@heplesser heplesser merged commit 445a7ba into nest:master Dec 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment