RFC: EnumSet type #19470

simonbyrne · 2016-12-01T13:29:36Z

Similar to an Enum, but represents a set of values via bitwise operations. Types themselves support certain bitwise operations (&, | and xor), and corresponding set operations.

stevengj · 2016-12-01T14:10:24Z

base/Enums.jl

+
+Base.union{T<:EnumSet}(x::T, y::T) = x | y
+Base.intersect{T<:EnumSet}(x::T, y::T) = x & y
+Base.issubset{T<:EnumSet}(x::T, y::T) = x & y == x


Would also be nice to be able to do EnumValue1 in x

I thought about that, but technically each "base member" is a singleton set, not an element. To make in mathematically correct we would need another type to distinguish these, which didn't seem worthwhile. Or we could just fudge it?

Mathematically, one should probably think of this as mereology. https://en.m.wikipedia.org/wiki/Mereology

So it is possibly acceptable for in to describe the mereologic relation.

Interesting: I like that idea.

I agree that in is too useful not to have. How about we rename it to FlagEnum, define in to be what is currently issubset, and get rid of all the set operations?

On second thought, issubset has a nice infix operator too, and does perhaps match closer intuition.

stevengj · 2016-12-01T14:11:26Z

+1 for the overall idea here. It's pretty common to have a bitwise-or of "flags", and it's nice to automate this.

simonbyrne · 2016-12-01T14:35:28Z

I've also added a demo of its use in the libgit2 bindings. Thoughts appreciated.

mauro3 · 2016-12-02T08:15:28Z

base/libgit2/consts.jl

- const CHECKOUT_CONFLICT_STYLE_DIFF3 = Cuint(1 << 21)
- const CHECKOUT_DONT_REMOVE_EXISTING = Cuint(1 << 22)
+ @enumset(CHECKOUT,
+ CHECKOUT_SAFE = 1 << 0,


Is it necessary to specify the values here?

Yes, as not all values are valid (e.g. 1 << 3).

Thanks, I didn't see that some where left out.

Some of the ones further down are unnecessary, but part of me thinks it might be good practice to leave them in for reference to the headers.

mauro3 · 2016-12-02T08:18:47Z

base/libgit2/types.jl

 pathspec::StrArrayStruct
 end
 StatusOptions(; show::Cint = Consts.STATUS_SHOW_INDEX_AND_WORKDIR,
- flags::Cuint = Consts.STATUS_OPT_INCLUDE_UNTRACKED |
+ flags::Consts.STATUS_OPT = Consts.STATUS_OPT_INCLUDE_UNTRACKED |


Indent on the following lines.

mauro3 · 2016-12-02T10:04:29Z

Would be good to add some docs which elaborate when to use @enum vs @enumset. Would EnumFlags be a better name?

simonbyrne · 2016-12-02T10:58:46Z

I agree, EnumFlag might be better choice.

simonbyrne · 2016-12-02T12:27:51Z

Also, need to decide whether to make the abstract type parametric, like Enum (#18826)

StefanKarpinski · 2016-12-02T23:15:24Z

I prefer EnumSet myself – it's a set of Enum values. To me a "flag" is a single value which can be included in a set of flags, so there isn't really a type that corresponds to the singular form – an "EnumFlag" is really just an EnumSet that happens to contain a single Enum value.

StefanKarpinski · 2016-12-02T23:17:01Z

Oh, I see, you're kind of using it that way since the "flag" term is used for the declaration of the individual flags that can go into a corresponding "enum set". Hmm...

nalimilan · 2016-12-03T10:43:21Z

The name choice is indeed difficult. EnumSet evokes a Set containing enum values at first sight. Maybe FlagEnum?

waldyrious · 2016-12-03T16:34:54Z

base/Enums.jl

+Create an [`FlagEnum`](:obj:`FlagEnum`) type with name `EnumName` and base member values of
+`EnumValue1` and `EnumValue2`, based on the unsigned integer type `U` (`UInt32` by
+default). The optional assigned values of `x` and `y` must have exactly 1 bit set, and
+not overlap. `EnumName` can be used just like other types and enum member values as


I'd rather phrase this as "If the values for x and y are provided, they must each have a single bit on, and naturally, not coincide".

The change "set" --> "on" is to prevent confusion with the noun meaning of "set", and the change "overlap" --> "coincide" is because when only one bit is turned on, conceptually we wouldn't consider it an overlap (which evokes partial coincidence); it either coincides exactly, or doesn't.

waldyrious · 2016-12-03T16:36:16Z

base/Enums.jl

+
+
+"""
+ @enumset EnumName[::U] EnumValue1[=x] EnumValue2[=y]


This should change to @flagenum, to match the rest of the code

waldyrious · 2016-12-03T16:37:22Z

base/Enums.jl

+regular values, such as
+
+```jldoctest
+julia> @enumset FRUITSET apple=1<<0 orange=1<<1 kiwi=1<<2


@flagenum here as well, and maybe rename FRUITSET to something else (FRUITFLAGS?)

waldyrious · 2016-12-03T16:38:18Z

test/enums.jl

@@ -169,3 +169,39 @@ let b = IOBuffer()
 seekstart(b)
 @test deserialize(b) === apple
 end
+
+
+@flagenum VegetableSet carrot potato broccoli


Should VegetableSet also be renamed, for consistency?

nalimilan · 2016-12-05T15:24:28Z

doc/stdlib/base.rst

@@ -700,6 +700,22 @@ Types
 julia> f(apple)
 "I'm a FRUIT with value: 1"

+.. function:: @flagenum FlagEnum[::U] EnumValue1[=x] EnumValue2[=y]


Should probably not use camel case for instances. Use lower case or upper case? Cf. #19506

tkelman · 2016-12-08T21:18:48Z

base/Enums.jl

+"""
+ @flagenum EnumName[::U] enumvalue1[=x] enumvalue1[=y]
+
+Create an [`FlagEnum`](:obj:`FlagEnum`) type with name `EnumName` and base member values


now should use @ref syntax for cross-references, but there's no docstring for FlagEnum

tkelman · 2016-12-08T21:19:27Z

base/exports.jl

@@ -1409,6 +1411,7 @@ export

 @assert,
 @enum,
+ @flagenum,


new exports need to be listed in a docs index to go into the new manual I believe

Yes, should be added after stdlib/base.md:114 where @enum's docstring is.

tkelman · 2016-12-08T21:31:44Z

doc/src/stdlib/base.md

@@ -112,6 +112,7 @@ Base.typejoin
 Base.typeintersect
 Base.Val
 Base.Enums.@enum
+Base.Enums.@flagenum


not this PR's fault, but this seems like it's in kind of an odd section

kshyatt · 2017-01-15T20:47:30Z

What is the status on this?

StefanKarpinski · 2017-01-26T23:09:26Z

I think we're stuck bikeshedding the name 😢

mauro3 · 2017-05-05T07:47:24Z

Had the need for this today. How about a 0.7 on this, so this doesn't fall through the cracks? +1 for FlagEnum or something else containing "flag".

JeffBezanson · 2017-07-08T20:16:35Z

This should probably be a package, as should the existing Enum type. Enums only seem to be used by Distributed and LibGit2, both of which should also be packages.

simonbyrne · 2017-07-08T21:56:13Z

Part of that reason is that it is due to (a) existing @enum doesn't support these sort of flags, and (b) it's loaded too late for a lot of things.

It could be useful in other places, e.g.

StefanKarpinski · 2017-08-31T18:34:52Z

Let's go ahead with this and call it EnumSet; it can have an EnumSet(::Enum) constructor so that you can use single Enum values where an EnumSet is expected and get the set containing only that value.

JeffBezanson · 2017-08-31T18:36:54Z

base/set.jl

@@ -107,7 +107,7 @@ function issubset(l, r)
 end
 const ⊆ = issubset
 ⊊(l::Set, r::Set) = <(l, r)
-⊈(l::Set, r::Set) = !⊆(l, r)
+⊈(l, r) = !⊆(l, r)


Does the new type support issubset?

StefanKarpinski · 2017-08-31T18:40:16Z

Jeff just pointed out that my last comment about conversion is incoherent since general enums have overlapping bits. But still, this should be considered as a set type with individual flags embedded as singleton sets.

StefanKarpinski · 2017-08-31T18:44:41Z

This is a feature and could be post-1.0; @JeffBezanson wants all of the things that use this to be removed from Base.

simonbyrne · 2017-08-31T20:33:53Z

You want to remove Libdl.open?

JeffBezanson · 2017-08-31T20:37:07Z

Yes, we can move Libdl out of Base.

simonbyrne · 2017-08-31T20:38:07Z

That was unhelpful, sorry. Actually after a bit more thought, I think we should move this out. We can always add the enum interfaces as extra.

JeffBezanson · 2017-09-14T18:26:28Z

+1 for EnumSet if we add this.

simonbyrne · 2017-12-14T20:05:37Z

So, I spent 10 minutes trying to rebase, but came to the realisation that I don't actually like it as it currently stands. A lot of enums mix "flag" parts and set parts (e.g. the open mode enum).

StefanKarpinski · 2017-12-14T20:29:52Z

Ok, in that case we can leave things as they are and if, in some 1.x version we introduce a better version of this feature, we can introduce new APIs that use enum sets while keeping the old integer-based ones around as legacy APIs. Not the end of the world.

stevengj reviewed Dec 1, 2016

View reviewed changes

mauro3 reviewed Dec 2, 2016

View reviewed changes

simonbyrne force-pushed the sb/enumset branch from be747da to cb15d61 Compare December 3, 2016 14:26

waldyrious reviewed Dec 3, 2016

View reviewed changes

nalimilan reviewed Dec 5, 2016

View reviewed changes

simonbyrne added 6 commits December 8, 2016 22:06

First draft of EnumSet

ac85147

use EnumSet for libgit2

d6ccaef

alignment

e3f79b1

rename to FlagEnum

19c88b7

update names

284afdc

change case

c39a6a2

simonbyrne force-pushed the sb/enumset branch from 1df3f29 to c39a6a2 Compare December 8, 2016 21:11

tkelman reviewed Dec 8, 2016

View reviewed changes

fix up for new docs

7f10b76

tkelman reviewed Dec 8, 2016

View reviewed changes

simonbyrne mentioned this pull request Jan 3, 2017

LibGit2 refactor #19839

Open

42 tasks

StefanKarpinski added this to the 1.0 milestone May 5, 2017

StefanKarpinski assigned simonbyrne Aug 31, 2017

JeffBezanson reviewed Aug 31, 2017

View reviewed changes

StefanKarpinski modified the milestones: 1.x, 1.0 Aug 31, 2017

JeffBezanson mentioned this pull request Sep 14, 2017

RFC: Deprecate open(filename) mode strings ("r", "w+", etc...) ? #14844

Closed

vtjnash added the status:triage This should be discussed on a triage call label Dec 13, 2017

vtjnash assigned vtjnash and unassigned vtjnash Dec 14, 2017

StefanKarpinski removed the status:triage This should be discussed on a triage call label Dec 14, 2017

DilumAluthge removed this from the 1.x milestone Mar 13, 2022

simonbyrne closed this Feb 13, 2023

simonbyrne deleted the sb/enumset branch February 13, 2023 17:19

RFC: EnumSet type #19470

RFC: EnumSet type #19470

Conversation

simonbyrne commented Dec 1, 2016

Choose a reason for hiding this comment

simonbyrne Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Dec 1, 2016

simonbyrne commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauro3 commented Dec 2, 2016

simonbyrne commented Dec 2, 2016 • edited Loading

simonbyrne commented Dec 2, 2016

StefanKarpinski commented Dec 2, 2016

StefanKarpinski commented Dec 2, 2016

nalimilan commented Dec 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waldyrious Dec 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshyatt commented Jan 15, 2017

StefanKarpinski commented Jan 26, 2017

mauro3 commented May 5, 2017

JeffBezanson commented Jul 8, 2017

simonbyrne commented Jul 8, 2017

StefanKarpinski commented Aug 31, 2017

Choose a reason for hiding this comment

StefanKarpinski commented Aug 31, 2017

StefanKarpinski commented Aug 31, 2017

simonbyrne commented Aug 31, 2017

JeffBezanson commented Aug 31, 2017

simonbyrne commented Aug 31, 2017

JeffBezanson commented Sep 14, 2017

simonbyrne commented Dec 14, 2017

StefanKarpinski commented Dec 14, 2017

simonbyrne Dec 1, 2016 •

edited

Loading

simonbyrne commented Dec 2, 2016 •

edited

Loading

waldyrious Dec 3, 2016 •

edited

Loading