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 `NumCell` with `NumericCellExt` for `Cell` #1064

Merged
merged 1 commit into from Jul 6, 2018

Conversation

Projects
None yet
4 participants
@ppannuto
Copy link
Member

ppannuto commented Jul 2, 2018

While I was reviewing #1046, I found myself confused at the mixture of Cell<usize> and NumCell<usize>, wondering if all usize things should be in NumCells?

Thinking about it more, it seemed like the continuation of NumCell reasoning would be a continuing array of Cell types for any specialized type, so I wondered if there might be a more 'rustic' way to do this, and it turns out extension types are a thing.

The idea here is that we can add the desired add and subtract methods directly to the original Cell type without creating a specialized NumCell type, and by the magic of trait bounds, the add and subtract methods will only work on types where it's meaningful to be able to add and subtract.

Currently we're only using NumCell in one place, so I switched that over to see what it would look like (this RFC).


The Question: What are our thoughts on extending the Cell type vs creating new types? I think extending Cell is cleaner and more extensible in the long run, but I could be missing drawbacks here.

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 2, 2018

I'm kind of in favor of removing NumCell, since it's just another type that has a pretty narrow use.

But I don't think we should go down the path of extension types until we have many examples that we can generalize from and come up with a good, consistent approach to.

@bradjc
Copy link
Contributor

bradjc left a comment

I think we should use abstractions to make code easier to read, and I find celltype.add(5) much easier to read than celltype.set(celltype.get() + 5), particularly with long variable names where that starts to wrap lines. And to the extent that we can use rust features to accomplish that I'm a big fan.

Side note: I have more commits that use numcell, they just are blocked on a chain of PRs at the moment.

tock-cells: Extension trait for numeric cells
Replace the specialized `NumCell` with an extension trait for `Cell`.

@ppannuto ppannuto force-pushed the numeric-cell branch from 6c058c5 to 19f2b58 Jul 3, 2018

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 3, 2018

I updated this to be a more complete implementation that would be viable to actually merge.

@ppannuto ppannuto removed the blocked label Jul 3, 2018

@phil-levis

This comment has been minimized.

Copy link
Collaborator

phil-levis commented Jul 4, 2018

I find celltype.add(5) much easier to read than celltype.set(celltype.get() + 5), particularly with long variable names where that starts to wrap lines. And to the extent that we can use rust features to accomplish that I'm a big fan.

Sure, I think that once you know what NumCell does and what is purpose is, it is simpler code than a regular Cell. But the tradeoff there is that you have to learn what NumCell does. So this is a tradeoff between making the code a little bit cleaner for an expert and a little bit less clear to a novice. I think self.val.add(5) is not a big cognitive cost, but I also think the gains are small. While understanding add() isn't hard, seeing a NumCell declared and understanding why means another type to understand.

That being said, it seems like if we want to have a Cell type for numbers, then we should think through exactly what its uses should be (I think we're pretty good on this one, so far), then based on that what its interface should be (we're only part-way there), then implement it and use it all places that we think match its intended use (not great, it's only used in one capsule). For example, why does NumCell support add and subtract but not shift or bitwise operations? Right now it seems like a narrowly designed abstraction that's useful now but will probably have to change as it sees more use, a class of abstractions I'm feeling pretty negative on right now.

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 4, 2018

but will probably have to change as it sees more use

It will probably have to expand, which is very different from changing. There is basically no cost, in terms of backward compatibility, to adding features to a type as needed.

As a user, you're not even going to have a field of type NumCellExt, you just have the option to impotr that trait and use it's methods on your Cell if your Cell contains a number.

The only way in which this might be a problem is if someone writes their own instance of NumCellExt, and would then have to implement additional methods. Though, with the implementation of NumCellExt for all Cell<Add+Substract> I really can't imagine a case for anyone implementing their own instance.

@ppannuto ppannuto changed the title [RFC] Extension trait for numeric cells Replace `NumCell` with `NumericCellExt` for `Cell` Jul 4, 2018

@ppannuto ppannuto removed the rfc label Jul 4, 2018

@bradjc

bradjc approved these changes Jul 5, 2018

Copy link
Contributor

bradjc left a comment

I think this is a really nice change. .add() is a very clear function.

@ppannuto ppannuto merged commit 9d453fa into master Jul 6, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ppannuto ppannuto deleted the numeric-cell branch Jul 6, 2018

@ppannuto ppannuto referenced this pull request Jul 6, 2018

Merged

fix dangling reference to NumCell #1078

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