Skip to content

Conversation

survivorm
Copy link
Contributor

No description provided.

@survivorm survivorm mentioned this pull request May 10, 2018

proc `*=`*[T, U: TimesMutableTypes](a: var T, b: U) =
a = a * b

Copy link
Contributor

@skilchen skilchen May 10, 2018

Choose a reason for hiding this comment

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

You can't multiply a TimesMutableTypes by another TimesMutableTypes. Thats why in my proposal, i only restricted the first generic type to types defined in times.nim. This restriction is enough to avoid the ambiguity with the mutators in system.nim. The remaining validation can be left to the operations defined in times.nim

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm. Valid point. But only for *=. I think other procs should remain as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@survivorm survivorm force-pushed the feature/times_mutators branch from 49f52e0 to c6761e7 Compare May 11, 2018 07:10
@survivorm
Copy link
Contributor Author

@dom96, or anyone else, what's with this request's merge?

@Araq
Copy link
Member

Araq commented Jun 4, 2018

@dom96, or anyone else, what's with this request's merge?

Well it has merge conflicts with the current devel version of times.nim, please rebase.

@survivorm
Copy link
Contributor Author

ok, will fix

@survivorm survivorm force-pushed the feature/times_mutators branch from c6761e7 to c025770 Compare June 5, 2018 09:02
@survivorm
Copy link
Contributor Author

@Araq Done

@Araq
Copy link
Member

Araq commented Jun 6, 2018

Er ok, but now it loses the nice runnableExamples (though they are currently not tested because they are in a generic, but still...).

@dom96
Copy link
Contributor

dom96 commented Jun 6, 2018 via email

@survivorm
Copy link
Contributor Author

@dom96 Do you have something SPECIFIC on your mind?
Because from my point of view it looks like: I don't have a decision, but this is incomplete (though current is incomplete too))

@survivorm survivorm force-pushed the feature/times_mutators branch from c025770 to b7a8eef Compare June 6, 2018 10:07
@survivorm
Copy link
Contributor Author

@Araq Added some examples

@dom96
Copy link
Contributor

dom96 commented Jun 6, 2018

What I specifically have in mind is a generic proc that works for all types that define +, *, etc. It should be defined in system.nim, this would ensure we don't have to define the mutator for every operator we define.

AFAIK @Araq was saying that + should be defined in terms of += for efficiency reasons. But this PR doesn't even do that, as such it's better to just implement what I'm proposing until we can figure out how to get the more efficient version (which seems rather tough).

@Araq
Copy link
Member

Araq commented Jun 6, 2018

AFAIK @Araq was saying that + should be defined in terms of += for efficiency reasons. But this PR doesn't even do that, as such it's better to just implement what I'm proposing until we can figure out how to get the more efficient version (which seems rather tough).

Don't let the perfect be an enemy of the good. And yet another system.nim change is not fine with me.

@survivorm
Copy link
Contributor Author

@dom96 Let us be clear.
If i understand correctly, you state, that no changes are better. Like it or not, current codebase has about 16 += procs as of today. We are not adding MORE, essentially. We're making them more generic, which i see as a better solution than it's now. Yes, it's not perfect. Yes, i'd like it completely generic, too. But while we don't have it, we may use something better than current. Partial, yes, but you'll still need to clear existing += and others of the like procs then the generic solution is ready. This PR is not adding more work.
About the @Araq 's proposition - i don't know it. If it helps, i may modify this PR accordingly

@survivorm
Copy link
Contributor Author

@Araq How to restart build checks? They've failed because of the strange bug, not connected with this PR

@dom96
Copy link
Contributor

dom96 commented Jun 6, 2018

Don't let the perfect be an enemy of the good. And yet another system.nim change is not fine with me.

Okay, but this isn't the first PR of this kind. We need to sort this out properly eventually.

@Araq Araq closed this Jun 6, 2018
@Araq Araq reopened this Jun 6, 2018
@survivorm
Copy link
Contributor Author

@Araq @dom96 Any more reasons not to merge this request?

@Araq Araq merged commit e06f5bc into nim-lang:devel Jun 7, 2018
@survivorm
Copy link
Contributor Author

@Araq @dom96 Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants