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

new outplace operator: doAssert @[2,1,3].>sort() == @[1,2,3] #13309

Closed
wants to merge 11 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Feb 1, 2020

  • this feature has often been requested
  • it avoids having to nest with parens, eg:
a.outplace(sort())
=>
a.>sort()

note

i was previously using .@

@Clyybber
Copy link
Contributor

Clyybber commented Feb 1, 2020

Nice, is the reason you are not using sugar.outplace directly here because of operator precedence?

If so, I think we generally should change operator precedence of things starting with one dot.
Starting with two dots: No change
Starting with one dot: Precedence same as the dot operator.

WDYT?

@Varriount
Copy link
Contributor

Personally I'd rather just use the function. The function isn't used enough to merit it's own operator

@disruptek
Copy link
Contributor

Agree, not a fan of adding linenoise. I’d rather wait and see if a convention evolves and then copy it.

Also, you have a typo in your doAssert message.

@timotheecour
Copy link
Member Author

timotheecour commented Feb 1, 2020

Personally I'd rather just use the function. The function isn't used enough to merit it's own operator

that's because it got recently introduced, and more importantly, a.outplace(sort()) is just too noisy / inconvenient to be worth it (longer + adds a parens nesting level).
With this PR, all you need to convert to outplace is add a @. Sometimes a bit of syntax sugar makes all the difference.

not a fan of adding linenoise. I’d rather wait and see if a convention evolves and then copy it.

not clear what you mean by adding linenoise. the noisier convention so far has been to add an overload with prefix d/ed, eg: sort => sorted, etc. @ is arguably better.

Also, you have a typo in your doAssert message.

fixed

Nice, is the reason you are not using sugar.outplace here because of operator precedence?

not sure I understand your comment. I am implementing .@ on top of outplace, and the rationale for .@ over .outplace(...) is convenience

@timotheecour timotheecour reopened this Feb 1, 2020
@Clyybber
Copy link
Contributor

Clyybber commented Feb 1, 2020

@timotheecour I meant to ask why .@ isn't a simple alias for outplace, but I see now that it just does some additional checking and stuff.

@timotheecour
Copy link
Member Author

mostly "and stuff": I needed to "rotate" the AST to make it work.
try

template `.@`*(lhs, rhs): untyped = outplace(lhs, rhs)

on the unittest examples and you'll see the problem.

@Araq
Copy link
Member

Araq commented Feb 3, 2020

2 ways to do the same are bad, we should only have .@ then and no "outplace" (it's a weird name anyway).

@Varriount
Copy link
Contributor

@Araq "outplace" is quite a bit more descriptive than @, which can easily be confused with the sequence operator

@timotheecour
Copy link
Member Author

timotheecour commented Feb 4, 2020

2 ways to do the same are bad, we should only have .@ then and no "outplace" (it's a weird name anyway).

PTAL:

  • I deprecated outplace and updated the tests
  • instead of using inplaceArgPosition (which would cause the exact same problems of adding parenthesis + making the call ugly), I found an even better way, using an optional _ placeholder:
    proc bar(x: int, ret: var int) = ret += x
    doAssert 3.@bar(4, _) == 3 + 4 # use placeholder `_` to specify a position > 0

see tests

@Araq
Copy link
Member

Araq commented Feb 4, 2020

I like it but we could have some bike-shedding session about the operator symbol to use.

@narimiran
Copy link
Member

narimiran commented Feb 4, 2020

no confusion possible with seq notation

This is a very bold claim.

I'll repeat my comments from IRC:


My concern is that @ now would mean two completely different things.

it's '.@' vs '@'

I know it is really a .@, but to an "untrained eye" it looks like @ following the usual dot.

How about .! ? Inspiration for .! are languages which have mutating! vs nonmutating, e.g. Julia.
To me foo.@bar(baz) would be confusing if i just came to Nim and learnt that $ converts to string, and @ converts to seq. and now somebody is converting a function to a seq?!?!

@kaushalmodi
Copy link
Contributor

While I like this operator, I am not so sure about the choice of @ symbol in there.

It kind of steals the special notation for sequences in Nim. So while I don't quite like .@ for this, it would be nice to have "some" operator (.! is a good candidate) instead of .outplace.

@Vindaar
Copy link
Contributor

Vindaar commented Feb 4, 2020

I already mentioned on IRC that I like .@ in principle, because of the familiarity to seq conversion, given that a majority of use cases will involve seqs. Thus it says something like "make me a temp seq here".

However, I do see the issue about confusing people. Given that Nim shouldn't become a "heres a * for you, have fun learning where it means what"-C, an alternative is probably better.

.! is nice, so I support that.

@Clyybber
Copy link
Contributor

Clyybber commented Feb 4, 2020

Its not really related to seqs, so .! might really be better to differentiate.
Or maybe .~ ?

@Araq
Copy link
Member

Araq commented Feb 4, 2020

Here is my current opinion: Let's move it to a new module, std / outplace and make it contain .@ or whatever operator we decide on. Maybe ./? Looks rather nice to me:

var a = @[1, 2, 3, 4, 5, 6, 7, 8, 9]
doAssert a./sort() == sorted(a)
#Chaining:
var aCopy = a
aCopy.insert(10)
doAssert a./insert(10)./sort() == sorted(aCopy)
doAssert @[1,3,2]./sort()./sort(order = SortOrder.Descending) == @[3,2,1]

@Clyybber
Copy link
Contributor

Clyybber commented Feb 4, 2020

With #13332 we can get rid of the AST "rotating".

@timotheecour
Copy link
Member Author

timotheecour commented Feb 4, 2020

Here is my current opinion: Let's move it to a new module, std / outplace and make it contain .@ or whatever operator we decide on. Maybe ./? Looks rather nice to me:

  • agree with moving to new module, making it usable in more places; but I've renamed it to outplaces to follow most common convention
  • added support for nnkCommand, eg: let ret = "ab".@add "cd"
  • changed .@ => ./
  • one last bikeshed: I'd be ok with ./ but:
    • it's reminiscant of path ./ separator, and is visually a bit confusing in things like doAssert "foo./bar///"./normalizePathEnd() == "foo./bar" ; I expect that path operations would be common in conjunction with ./ (to outplace various inplace operators in std/strutils or std/os)
    • in particular it's same as import ./mymod but does NOT causes actual compiler ambiguities (side issue: import ./mymod should have meaning of forcing the import to be in same directory, but currently doesn't enforce that)
    • it could cause confusion with a linear algebra operator, eg in matlab/octave, a./b does element-wise division of matrices a and b; likewise with .*, etc

alternative symbol

The other suggestion i had was: .^(also exists in matlab/octave as element-wise power of a to the b, but arguably more rare); rationale:

  • indicates insertion point (eg in clang err messages)
  • less likely to cause visual confusion (clashes with rarer things like exponent or XOR)

With #13332 we can get rid of the AST "rotating".

I suppose we can still merge this and update code if/when yours gets merged

@timotheecour
Copy link
Member Author

timotheecour commented Feb 4, 2020

@Araq PTAL

  • see above remarks
  • plus, removed outplace
  • plus, changed to .> which was recommended to me and which i like (and indicates insertion etc)

@c-blake
Copy link
Contributor

c-blake commented Feb 4, 2020

I also liked .!. E.g., a.!insert(10).!sort(). Looks nice with or without extra spaces. a .! insert(10) .! sort()

@Araq
Copy link
Member

Araq commented Feb 4, 2020

I think .> is the winner.

@kaushalmodi
Copy link
Contributor

@timotheecour Now you just need to edit this PR's title 😁

Thanks for working on this feature!

@timotheecour timotheecour changed the title new outplace operator: doAssert @[2,1,3].@sort() == @[1,2,3] new outplace operator: doAssert @[2,1,3].>sort() == @[1,2,3] Feb 4, 2020
Copy link
Contributor

@dom96 dom96 left a comment

Choose a reason for hiding this comment

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

Since this is now in a separate module, can't we check how popular this is as a nimble package first? Then decide whether to include it in the stdlib or not?

We need less of these tiny modules that only 1% of our already small user base are using. It's unrealistic for us to maintain.

## Outplace operator: turns an `in-place`:idx: algorithm into one that works on
## a copy and returns this copy. A placeholder `_` can optionally be used to
## specify an output parameter of position > 0.
## **Since**: Version 1.2.
Copy link
Contributor

Choose a reason for hiding this comment

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

Paragraphs! Please add empty line above this, otherwise it'll render poorly in the doc gen.

Also, you need to add this file to the docgen config IIRC.

Copy link
Member Author

@timotheecour timotheecour Feb 4, 2020

Choose a reason for hiding this comment

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

Please add empty line above this

done

Also, you need to add this file to the docgen config IIRC.

I believe I've fixed this in a general way in #13221

@timotheecour
Copy link
Member Author

timotheecour commented Feb 5, 2020

Since this is now in a separate module, can't we check how popular this is as a nimble package first? Then decide whether to include it in the stdlib or not?
We need less of these tiny modules that only 1% of our already small user base are using. It's unrealistic for us to maintain.

I fail to see see how it's unrealistic to maintain this, let's not blow things out of proportion or use FUD.

see note from @Araq https://irclogs.nim-lang.org/23-01-2020.html#13:43:07

our guidelines need to be expanded, we need a rule like
common syntactic sugar should be in the stdlib

that's to ensure uniformity instead of alternatives where you'd have import pkg/outplaces and then later when added, import std/outplaces causing conflicts (and definitely more maintenance hurdle down the road);
furthermore that'd prevent its use in stdlib + compiler sources which is a prime target for benefiting from this feature.
This issue bw inplace vs outplace is almost everywhere, status quo so far was to either have convenience but maybe slow, fast but maybe inconvenient, or have source code bloat:

  • strutils.addSep, trimZeros, addf, stripLineEnd, delete, removePrefix, removeSuffix etc would all benefit from it;

currently we have a rather arbitrary mix (and that's just for stdlib):

  • some API's have a single outplace variant,
  • some API's have a single inplace variant,
  • some API's have both, with the outplace for foo named food or fooed depending on grammar
  • some some API's have both, with same name.

After this PR the new guidelines are simple:

  • only ever write outplace variant where that variant makes sense to have for efficiency
  • callers can choose foo(arg, ...) or arg.>foo(...) depending on whether inplace or outplace is needed.

@timotheecour
Copy link
Member Author

please do not merge yet

@Varriount
Copy link
Contributor

Is there any way this could optimize consecutive uses of the operator, to prevent spurious intermediate copies from being made?

@Clyybber
Copy link
Contributor

Clyybber commented Feb 5, 2020

@Varriount They will be optimized to moves anyways, but yeah. Timothee is working on exactly that.

@Araq
Copy link
Member

Araq commented Feb 5, 2020

Since this is now in a separate module, can't we check how popular this is as a nimble package first? Then decide whether to include it in the stdlib or not?
We need less of these tiny modules that only 1% of our already small user base are using. It's unrealistic for us to maintain.

If you're after honest numbers there is no way to measure it but to include it into the core and declare it to be "experimental". And then later on you can see how often if it's used or not. People avoid dependencies like the plague and even moreso for "tiny syntactic improvements". I wouldn't use package / outplace in Karax. I would use std / outplace in Karax. Others have the same attitude (eg. @alehander92 ), no matter how good Nimble is at dependency resolution.

@timotheecour
Copy link
Member Author

@Varriount They will be optimized to moves anyways, but yeah. Timothee is working on exactly that.

actually they won't be optimized away, it's easy to check by inserting addr in the right spots; however, I've done precisely that in #13337 while everyone is busy bikeshedding about which operator to use ;-) (jk); will merge back here once it's ready

@alehander92
Copy link
Contributor

sorry there was a misunderstanding, yes i agree with Araq for simple sugar, but not for complicated like pattern matching: not sure where does this feature fall into

@timotheecour
Copy link
Member Author

timotheecour commented Feb 5, 2020

Out of curiosity, does it still need to be called "outplaces" instead of just "outplace"?

to follow a not-always-but-often observed convention that module name use plural; I'd be ok with outplace if really needed but I'd prefer to stick to that convention, which avoids name conflict issues (even if no outplace symbol ends up being defined; it's just easier to follow simple conventions)

@c-blake
Copy link
Contributor

c-blake commented Feb 5, 2020

Only 30/84 of the modules at the top level of lib/pure are plural. Doubt anyone will want "outplace" singular or plural for anything else. Don't really care myself. Just adding some easily had data.

@Clyybber
Copy link
Contributor

Clyybber commented Feb 5, 2020

Plural doesn`t make sense in this case IMO.
Convention is to use what makes sense :p

@kaushalmodi
Copy link
Contributor

I am looking forward to seeing this merged. @Araq the only resolution needed here is now outplaces module vs outplace?

@Araq
Copy link
Member

Araq commented Feb 14, 2020

Depends on nim-lang/RFCs#192

@narimiran
Copy link
Member

the only resolution needed here is...

nim-lang/RFCs#192

outplaces module vs outplace

FWIW, outplaces makes no sense to me.

@Araq
Copy link
Member

Araq commented Apr 20, 2020

Outdated PR, nim-lang/RFCs#192 is shipping.

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.