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 Enum.SetFlag and Enum.RemoveFlag to make bitwise flagging easier #14084

Closed
yufeih opened this issue Feb 8, 2015 · 33 comments
Closed

Add Enum.SetFlag and Enum.RemoveFlag to make bitwise flagging easier #14084

yufeih opened this issue Feb 8, 2015 · 33 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Milestone

Comments

@yufeih
Copy link
Contributor

yufeih commented Feb 8, 2015

To maintain multiple bool states in a class, instead of declaring many bool fields, an enum flag can be used to compact these states in to a single member to optimize for memory usage.

[Flags] enum DirtyFlags { X = 1, Y = 1 << 1, Z = 1 << 2 };

DirtyFlags dirtyFlags = DirtyFlags.X | DirtyFlags.Y | DirtyFlags.Z;

It is simple to test if a flag is set using Enum.HasFlag(DirtyFlags.X), but the scenario is not complete. Without the ability to set and remove a flag, the following code needs to be maintained manually:

// Setting a flag
dirtyFlags |= DirtyFlags.X;
// Removing a flag, this is harder to remember :(
dirtyFlags &= ^DirtyFlags.X;

The suggestion is to add 2 methods to Enum to help set and clear a flag.

public bool HasFlag(Enum flag); // This method already exists
public bool SetFlag(Enum flag);
public bool RemoveFlag(Enum flag);

These methods should be aggressively inlined since bitwise manipulations is very likely to be used in a performance sensitive scenario.

@mikedn
Copy link
Contributor

mikedn commented Feb 8, 2015

With the current language and runtime capabilities there's no way to implement SetFlag and RemoveFlag efficiently. Even the existing HasFlag has horrible performance compared to the "manual" way of testing flags.

@Alexx999
Copy link
Contributor

Alexx999 commented Feb 9, 2015

@yufeih enum is now implemented in, well, not nice way. It's half-struct half-class which can turn out be any type in practice, and framework uses tons of hacks and quirks to make it work like it should.
So @mikedn is probably right - it will be hard to make it work fast.
However, it's probably worth trying.

UPD: Okay, looked at it for a bit more... I believe not much can be done here without changing specifications for enum. Now, argument for HasFlags is treated as object, so it requires boxing-unboxing and has corresponding performance hit.
It will not work fast without some serious changes in how enums work (generics?) but that will probably require changes at runtime and/or compilers.

@HellBrick
Copy link

This is another example where enum constraint on a generic argument (dotnet/roslyn#262) would be useful. It would allow something like

T WithFlags<T>( this T source, T flagsToAdd ) where T: enum { /* ... */ }
T WithoutFlags<T>( this T source, T flagsToRemove ) where T: enum { /* ... */ }

@HaloFour
Copy link

HaloFour commented Feb 9, 2015

@HellBrick Even with the enum constraint the problem is that the underlying type could be anything from sbyte to ulong and the IL for the bitwise math would have to be different for each of those types.

Some of this came up on the CodePlex Roslyn forums when I proposed supporting an enum constraint for C# generics with support for | and &.

@mikedn
Copy link
Contributor

mikedn commented Feb 9, 2015

sbyte to ulong and the IL for the bitwise math would have to be different for each of those types

As I explained before, the bitwise operations themselves are fine. The problem is with conversions that are needed in certain cases. That's something that could probably be fixed in the runtime, at least in the specific case of enums.

@terrajobst
Copy link
Member

@mikedn

With the current language and runtime capabilities there's no way to implement SetFlag and RemoveFlag efficiently. Even the existing HasFlag has horrible performance compared to the "manual" way of testing flags.

I agree with this with why I believe we shouldn't add these APIs.

@weshaggard @KrzysztofCwalina what are your thoughts?

@yufeih
Copy link
Contributor Author

yufeih commented Jan 5, 2016

I can see how HasFlags is implemented in a really inefficient way. These methods should not be added like that. Maybe some kind of JitIntrinsic from coreclr might help here?
If the effort is not worth the outcome, I am fine with leaving things as is.

@KrzysztofCwalina
Copy link
Member

@terrajobst, I agree that we don't want to simply add these APIs, but I would put this (efficient flags/bitwise operations) issue on our list of big ticket items that we would like to consider fixing as part of the "modern BCL" push.

@HaloFour
Copy link

HaloFour commented Jan 5, 2016

It would make a lot more sense to wait until C# supported generic enum constraints:

dotnet/roslyn#262

@KrzysztofCwalina
Copy link
Member

I am not sure how enum constraint would help. The type system still needs to describe the size of the backing integral type to implement many of these operations efficiently. I think something like making enums extend Enum where T: integral type would be needed.

@HaloFour
Copy link

HaloFour commented Jan 5, 2016

@KrzysztofCwalina True. Not perfect, but wouldn't an unchecked cast to ulong and then back to the enum type be more efficient than what we have today?

@terrajobst
Copy link
Member

Not sure that would work in all cases either. What if the enum is backed by a double/float? C# doesn't support CLI does.

@kornman00
Copy link

@terrajobst is there online documentation covering that fact (CLI supporting non-integral backing types for enums)? Not finding any obvious results with my Google-fu

@mikedn
Copy link
Contributor

mikedn commented Mar 6, 2016

is there online documentation covering that fact (CLI supporting non-integral backing types for enums)? Not finding any obvious results with my Google-fu

Hmm, didn't notice this comment before. There's no such enum, CLI supports only enums of integral type:

The symbols of an enum are represented by an underlying integer type: one of { bool, char, int8, unsigned int8, int16, unsigned int16, int32, unsigned int32, int64, unsigned int64, native int, unsigned native int }

The difference between what CLI supports and what C# supports is { bool, char, native int, native unsigned int }

@kornman00
Copy link

@mikedn Ah, thanks for the clarification! Are you aware of any CLI-based languages that do allow those underlying types in their spec?

@mikedn
Copy link
Contributor

mikedn commented Mar 6, 2016

Are you aware of any CLI-based languages that do allow those underlying types in their spec?

Nope. But it's not very relevant as it's conceivable that C# would support those types in a future version so it's something that one needs to consider. But those additional type can be casted to long just like the supported types so it doesn't really matter.

That said, casting to long is AFAIR not enough to solve this problem. You can cast to long (though ulong might be a better choice) just fine and then you can set/reset the flag. But then you need to cast back to the original enum underlying type and that's problematic in generic code.

@kornman00
Copy link

FWIW, I solved this problem by using LINQ to generate the type-safe Set/Remove/Test flags methods -

V1: Converts the enum values to their underlying types, performs bitwise ops, then converts back to enum type.

V2: Performs the bitwise ops directly on the enum's backing 'value__' field.

Unit tests

@mikedn
Copy link
Contributor

mikedn commented Mar 6, 2016

FWIW, I solved this problem by using LINQ to generate the type-safe Set/Remove/Test flags methods

Not really. Expressions are interpreted rather than compiled in AOT environments such as .NET Native. And even if expressions are compiled the call through delegate cost is much higher than the cost of a "normal" implementation.

@TylerBrinkley
Copy link
Contributor

I apologize for the shameless plug but I believe this is pertinent to users. I've just released version 1.0.0 of Enums.NET, a high-performance type-safe .NET enum utility library.

It has the extension methods CombineFlags and RemoveFlags which are constrained to the enum type which addresses this issue.

Enums.NET is available on NuGet and is compatible with .NET Standard 1.0+ as well as .NET Framework 2.0+.

@TylerBrinkley
Copy link
Contributor

I've created a formal API proposal dotnet/corefx#15453 that addresses this request.

@ghost
Copy link

ghost commented Feb 14, 2018

I wrote this class and it works:

Public Class Flags
    Public Shared Function SetFlags(value As [Enum], ParamArray flags() As [Enum]) As [Enum]
        If flags.Length = 0 Then Throw New Exception("Please send at least one flag")
        For Each flag In flags
            value = CObj(value) Or CObj(flag)
        Next
        Return value
    End Function

    Public Shared Function ResetFlags(value As [Enum], ParamArray flags() As [Enum]) As [Enum]
        If flags.Length = 0 Then Throw New Exception("Please send at least one flag")
        For Each flag In flags
            value = CObj(value) And Not CObj(flag)
        Next
        Return value
    End Function

    Public Shared Function IsSet(value As [Enum], flag As [Enum]) As Boolean
        Return value.HasFlag(flag)
    End Function
End Class

The Enum Class can have the SetFlage and RemoveFlag in the same way.

@mikernet
Copy link
Contributor

mikernet commented Nov 22, 2019

There's no need for crazy complicated code to make generic enum methods work - Unsafe can be used to build very fast generic enum methods that run at full bitwise operator speed via the following technique:

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static unsafe bool HasAllFlags<T>(T value, T flags) where T : unmanaged, Enum
        {
            if (sizeof(T) == 1)
                return (Unsafe.As<T, byte>(ref value) | Unsafe.As<T, byte>(ref flags)) == Unsafe.As<T, byte>(ref value);
            else if (sizeof(T) == 2)
                return (Unsafe.As<T, short>(ref value) | Unsafe.As<T, short>(ref flags)) == Unsafe.As<T, short>(ref value);
            else if (sizeof(T) == 4)
                return (Unsafe.As<T, int>(ref value) | Unsafe.As<T, int>(ref flags)) == Unsafe.As<T, int>(ref value);
            else // size == 8
                return (Unsafe.As<T, long>(ref value) | Unsafe.As<T, long>(ref flags)) == Unsafe.As<T, long>(ref value);
        }

A similar pattern can be applied for other operations. The conditional branches are eliminated by the JIT.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@joperezr joperezr removed the untriaged New issue has not been triaged by the area owner label Jul 6, 2020
@danmoseley
Copy link
Member

This isn't required to ship 5.0

@adamsitnik
Copy link
Member

@danmosemsft Is there any chance we could introduce this API in 6.0? I find myself implementing it in every project I use ;)

@danmoseley
Copy link
Member

@adamsitnik if you are passionate about it your next step would be to get to api-ready-for-review and represent at API review.

@zgabi
Copy link
Contributor

zgabi commented Dec 19, 2020

There should be a non-generic version of the SetFlag/WithFlags/RemoveFlags/WithoutFlags, too.

For example:
public static object SetFlag(Type enumType, object value, object flag)
Or with ref parameter.

Similar to the Parse/TryParse methods: #14083

@mburbea
Copy link

mburbea commented Mar 21, 2021

I would propose the following api to solve this:

public class Enum: ValueType
{
+public T SetFlag<T>(T flag, bool condition = true) where T:struct,Enum;
+public T UnsetFlag<T>(T flag) where T:struct,Enum => SetFlag(flag, false);
}

Edit: @danmoseley, anything else needed before this api could be ready for review?

I wrote this as an extension recently while updating a wpf application that bound several checkboxes to a single flagged enum. This made the properties I bound to on my object model nice and symmetric.
e.g.

        public bool IsStolen
        {
            get => _flags.HasFlag(InventoryFlags.IsFromStolenSource);
            set => _flags = _flags.SetFlag(InventoryFlags.IsFromStolenSource, value);
        }

Unfortunately the implementation itself was super hideous.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static T SetFlag<T>(this T @enum, T flag, bool on) where T : struct, Enum
{
    if (Unsafe.SizeOf<T>() == 1)
    {
        byte x = (byte)((Unsafe.As<T, byte>(ref @enum) & ~Unsafe.As<T, byte>(ref flag))
            | (-Unsafe.As<bool, byte>(ref on) & Unsafe.As<T, byte>(ref flag)));
        return Unsafe.As<byte, T>(ref x);
    }
    else if (Unsafe.SizeOf<T>() == 2)
    {
        var x = (short)((Unsafe.As<T, short>(ref @enum) & ~Unsafe.As<T, short>(ref flag))
            | (-Unsafe.As<bool, byte>(ref on) & Unsafe.As<T, short>(ref flag)));
        return Unsafe.As<short, T>(ref x);
    }
    else if (Unsafe.SizeOf<T>() == 4)
    {
        uint x = (Unsafe.As<T, uint>(ref @enum) & ~Unsafe.As<T, uint>(ref flag))
           | ((uint)-Unsafe.As<bool, byte>(ref on) & Unsafe.As<T, uint>(ref flag));
        return Unsafe.As<uint, T>(ref x);
    }
    else
    {
        ulong x = (Unsafe.As<T, ulong>(ref @enum) & ~Unsafe.As<T, ulong>(ref flag))
           | ((ulong)-(long)Unsafe.As<bool, byte>(ref on) & Unsafe.As<T, ulong>(ref flag));
        return Unsafe.As<ulong, T>(ref x);
    }
}

@NN---
Copy link
Contributor

NN--- commented Mar 22, 2021

@hopperpl
Copy link

What is the state of this now? Before I start yet another round of API suggestions for Enum.HasAnyFlag() et al.

Enum.HasFlag is now inlined without performance penalty and it makes code look better, imo. And as a bonus in DEBUG it comes with a type check. Set/Unset/Has/HasAny will also avoid endless castings to/from int as the compiler is not smart enough to do it automatically.

This is 6.5 years old. There should be a decision now if it will be added or dropped.

@tannergooding
Copy link
Member

@jkotas, any concerns on adding additional APIs to System.Enum here. It sounds like there would be 1-3 APIs and there has been continued interest since the initial proposal 7 years ago.

The three core scenarios are "set", "clear", and "toggle". Which could be handled via a single API:

public static TEnum SetFlag<TEnum>(TEnum value, TEnum flag, bool value);

or a set of APIs:

public static TEnum ClearFlag<TEnum>(TEnum value, TEnum flag);
public static TEnum SetFlag<TEnum>(TEnum value, TEnum flag);
public static TEnum ToggleFlag<TEnum>(TEnum value, TEnum flag);

names up for debate, as would be whether they are instance or static.

@danmoseley
Copy link
Member

cc @stephentoub here, since as I recall in a previous issue, he argued against providing alternative syntactic sugar to do basic flags operations. Instead we should just make sure that the standard way of writing such things (which is not C# specific) is efficient. I have to say I agree. When I see one of these methods, I would wonder which one to use.

Apologies for overlooking the ping to me higher up.

@stephentoub
Copy link
Member

We've closed other related proposals, e.g. #66261. @jkotas has also expressed an opinion similar to mine, e.g. #55455 (comment), and also highlighted that just doing the idiomatic bit operations are guaranteed to be efficient where such named methods aren't, e.g. #55455 (comment).

@tannergooding
Copy link
Member

Closing as per the comments above.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-System.Runtime
Projects
None yet
Development

No branches or pull requests