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

add setutils.[]= #17272

Merged
merged 4 commits into from
Mar 8, 2021
Merged

add setutils.[]= #17272

merged 4 commits into from
Mar 8, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Mar 6, 2021

now that we have a dedicated std/setutils, it makes sense to add this syntax sugar, which is useful in particular when the value (true or false) isn't hard-coded, thus avoiding if ... incl else ... excl

future work

@timotheecour timotheecour requested a review from ringabout March 6, 2021 03:06
Copy link
Member

@ringabout ringabout left a comment

Choose a reason for hiding this comment

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

It is not very intuitive, do other language have similar features?

@metagn
Copy link
Collaborator

metagn commented Mar 6, 2021

It is not very intuitive, do other language have similar features?

http://www.cplusplus.com/reference/bitset/bitset/operator[]/

@timotheecour
Copy link
Member Author

timotheecour commented Mar 6, 2021

in D:

https://dlang.org/library/std/bitmanip/bit_array.html

import std.bitmanip;
void main(string[]args){
  auto b = BitArray([1, 0]);
  assert(b[0] == true);
  b[0] = false;
  assert(b[0] == false);
}

in C++

#include <bitset>
#include <cassert>
int main (int argc, char *argv[]) {
  std::bitset<128> a;
  a[3] = true;
  assert(a[3] == true);
  return 0;
}

in swift:

https://victorqi.gitbooks.io/swift-algorithm/content/bit_set.html

var bits = BitSet(size: 140)
bits[2] = true

lib/std/setutils.nim Show resolved Hide resolved
lib/std/setutils.nim Show resolved Hide resolved
tests/stdlib/tsetutils.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_setutils_set_elem branch from 065518e to 518eac7 Compare March 6, 2021 23:36
@beef331
Copy link
Collaborator

beef331 commented Mar 7, 2021

I do like the feature but really dislike the syntax, I'm suggesting s.set(a3, true), though it doesn't match the other languages but is a whole hell of a lot more readable to me.

@timotheecour
Copy link
Member Author

timotheecour commented Mar 7, 2021

I do like the feature but really dislike the syntax, I'm suggesting s.set(a3, true), though it doesn't match the other languages but is a whole hell of a lot more readable to me.

well []= would fit well with Tables, etc, but I'm open to a different name. set is bad though since it's even more confusing, because set itself is the type name. However put is the other obvious choice, and also is mentioned for that in nep1 (https://nim-lang.github.io/Nim/nep1.html)

vote as reaction to this comment:

  • +1 to keep []=
  • -1 for put

otherwise, add a comment for a different suggestion.
(note that future work may add an overload taking a set as 2nd param, eg; s.put({a0, a1}, false) vs s[{a0, a1}] = false depending on the choice made here)

lib/std/setutils.nim Outdated Show resolved Hide resolved
@timotheecour timotheecour force-pushed the pr_setutils_set_elem branch from 518eac7 to 074c8a9 Compare March 7, 2021 05:06
@Araq Araq merged commit 93cb5d6 into nim-lang:devel Mar 8, 2021
@timotheecour timotheecour deleted the pr_setutils_set_elem branch March 8, 2021 18:57
@timotheecour timotheecour added TODO: followup needed remove tag once fixed or tracked elsewhere and removed TODO: followup needed remove tag once fixed or tracked elsewhere labels Mar 8, 2021
ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
* add setutils.[]=
* address comments
* proc => func (for other symbols too)
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* add setutils.[]=
* address comments
* proc => func (for other symbols too)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants