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

STL compatible size(), dimensions() return size_t #357

Open
breznak opened this issue Apr 3, 2019 · 5 comments
Open

STL compatible size(), dimensions() return size_t #357

breznak opened this issue Apr 3, 2019 · 5 comments
Labels
code code enhancement, optimization, cleanup..programmer stuff question Further information is requested

Comments

@breznak
Copy link
Member

breznak commented Apr 3, 2019

A coding-style proposal:
our new code will be STL compatible and size(), dimensions(), capacity, etc will return size_t
value;

Current:
we use UInt or certain type that is used in the container (eg vector<char>()::size() we'd cast to char.

New: STL uses size_type (size_t, ulong)

Motivation: in PR #331 I'm playing with changing back-end data-types for containers. We get incompatibilities with other parts in the code on each change, deciding on size_t would fix this.

@breznak breznak added question Further information is requested code code enhancement, optimization, cleanup..programmer stuff labels Apr 3, 2019
@ctrl-z-9000-times
Copy link
Collaborator

This sounds great. Its always nice to be compatible with the standard library.

@dkeeney
Copy link

dkeeney commented Apr 4, 2019

I agree. Anything related to size ... or indexes for that matter, should be size_t. There is a nupic::Size type but it is not consistently used.

@breznak
Copy link
Member Author

breznak commented Apr 4, 2019

There is a nupic::Size type but it is not consistently used.

good point, should we use nupic::Size (i think now typedef'ed to size_t), or just drop it and deal with size_t only?

@dkeeney
Copy link

dkeeney commented Apr 4, 2019

or just drop it and deal with size_t only?

Well, for what it is worth, Cereal uses an unsigned long long int for size rather than size_t. But I think that is a spacial case and everything else uses size_t.

Let's just use size_t.

@breznak
Copy link
Member Author

breznak commented Apr 7, 2019

from #372 (comment) , @dkeeney

To be consistent....we should make ALL size related things size_t. This includes the dimensions.

yes, although I'd like to advocate against that.

  • making dimensions size_t turned out a pain, and I messed up with SDR, SdrTools
  • more importantly, semantics and ranges:
    • dimensions dictate size of (number of) cells (CellIdx), so it makes sense dimensions be CellIdx
      • dimensions is an input arg, CellIdx does not allow the user to set a number out of range
    • size_t is (typically) used for return types (size_t getNumSynapses()) with motivation to allow toggling types in Connections

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code code enhancement, optimization, cleanup..programmer stuff question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants