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 enumutils.items for sparse enums, typetraits.SomeSparseEnum #17080

Merged
merged 14 commits into from
Feb 23, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Feb 18, 2021

(inspired by enmRange in #17066, thanks @beef331 !)

future work

  • I suggest changing holey enum in manual (and doc comments etc) to sparse enum, IMO it's just better. No code would break since (until this PR) we dont' have public API's mentioning hole/holey (IIRC); EDIT: after review comments, I changed it back to enum with holes
  • genEnumCaseStmt needs tests and runnableExamples
  • proc nativeString*(a: enum): string {.compileTime.} magic proc which would by pass enum string definitions:
type Foo = enum
  kf0 = "F0"
  kf1 = "F1"
var a = kf0
doAsert $a == "F0"
doAsert a.nativeString == "kf0"

@beef331
Copy link
Collaborator

beef331 commented Feb 18, 2021

Probably should use holey and not the religious holy :D

@timotheecour
Copy link
Member Author

Probably should use holey and not the religious holy :D

good point; come to think of it, sparse enum is IMO a better terminology; since we don't yet have API's with holey, I'm changing terminology to sparse instead of enum; could be generalized in doc comments in future work

@timotheecour timotheecour changed the title add enumutils.items for enum with holes, typetraits.SomeHolyEnum add enumutils.items for sparse enums, typetraits.SomeSparseEnum Feb 18, 2021
changelog.md Outdated Show resolved Hide resolved
lib/std/enumutils.nim Outdated Show resolved Hide resolved
lib/pure/typetraits.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

PTAL @hlaaftana

lib/pure/typetraits.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

refs #17080 (comment) please vote by adding +1 or -1 reaction to this msg (not the PR itself) as follows:

  • +1 to change terminology to sparse enum
  • -1 to keep terminology of holey enum or enum with holes

the rationale for changing terminology is provided here #17080 (comment) and (as i mentioned) isn't a breaking change since no API uses this term yet (until this PR, that is)

@Araq
Copy link
Member

Araq commented Feb 22, 2021

The terminology "enums with holes" is entirely justified: It's precise and does convey the idea that they are defective: You cannot use these enums as array indexes, the code generation for case e suffers, the storage can easily exceed the otherwise required one byte. "sparse" implies none of these things, IMO.

doc/lib.rst Outdated Show resolved Hide resolved
lib/pure/typetraits.nim Outdated Show resolved Hide resolved
lib/std/enumutils.nim Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

PTAL

The terminology "enums with holes" is entirely justified: It's precise and does convey the idea that they are defective: You cannot use these enums as array indexes, the code generation for case e suffers, the storage can easily exceed the otherwise required one byte. "sparse" implies none of these things, IMO.

I've changed it back to enum with holes for the sake of moving on but I would've preferred sparse enum, it's more standard and in the context of enum's conveys the same thing.

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Feb 23, 2021
@Araq Araq merged commit c274e67 into nim-lang:devel Feb 23, 2021
@timotheecour timotheecour deleted the pr_items_enumwithholes branch February 23, 2021 19:35
ringabout pushed a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
…lang#17080)

* add enumutils.items for enum with holes
* changelog
* ref in lib.rst
* use `type SomeSparseEnum* = (not Ordinal) and enum` instead of concept
* address comment: rename back to enum with holes
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
…lang#17080)

* add enumutils.items for enum with holes
* changelog
* ref in lib.rst
* use `type SomeSparseEnum* = (not Ordinal) and enum` instead of concept
* address comment: rename back to enum with holes
@doongjohn
Copy link

import std/enumutils

type Color = enum  Red = 0, Green = 2, Blue = 3

for c in Color.items:
  echo c

compile output of this code:

/home/****/main.nim(10, 15) template/generic instantiation of `items` from here
/usr/lib/nim/std/enumutils.nim(97, 25) template/generic instantiation of `items` from here
/usr/lib/nim/system/iterators.nim(54, 9) Warning: conversion to enum with holes is unsafe: T(i) [HoleEnumConv]
/home/****/main.nim(10, 15) template/generic instantiation of `items` from here
/usr/lib/nim/std/enumutils.nim(97, 25) template/generic instantiation of `items` from here
/usr/lib/nim/system/iterators.nim(54, 26) Warning: conversion to enum with holes is unsafe: T(i) [HoleEnumConv]
Red
Green
Blue

isn't enumutils.items safe to use with Holey Enum?

@Araq
Copy link
Member

Araq commented Nov 10, 2021

Enumutils.items should use cast, I guess.

@Araq
Copy link
Member

Araq commented Nov 11, 2021

Ping @timotheecour

@timotheecour
Copy link
Member Author

that's just a symptom; the root cause is elsewhere, IMO:

when true:
  type Color = enum  Red = 0, Green = 2, Blue = 3
  let s = {Red, Green, Blue}
  for si in s: echo si

also gives this warning, even though it doesn't use std/enumutils (note that enumutils itself creates a set via enumWithHolesFullRange)

warning goes away with this diff:

diff --git a/lib/system/iterators.nim b/lib/system/iterators.nim
index f23f7aa11..4b82f9235 100644
--- a/lib/system/iterators.nim
+++ b/lib/system/iterators.nim
@@ -51,7 +51,7 @@ iterator items*[T](a: set[T]): T {.inline.} =
   ## able to hold).
   var i = low(T).int
   while i <= high(T).int:
-    if T(i) in a: yield T(i)
+    if cast[T](i) in a: yield cast[T](i)
     inc(i)

but TBD whether this is a good diff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere Vote
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enums with holes are iterable
8 participants