Color struct defines 1 `uint` field, and many consts typed as `int`

[AArnott]

Why does the GDI+ Color struct in the metadata define a uint instance field and many int constants that would appear to be meant to be assigned to the instance field?

    public uint Argb;
    public const int AliceBlue = -984833;
    public const int AntiqueWhite = -332841;
    // ...

I further wonder why these constants are not typed as Color themselves, similar to how S_OK is defined with a value of 0 (the integer) but typed as HRESULT?

Ideally we want the user to be able to type Color.AliceBlue and have that represent an instance of the Color struct initialized to the value in AliceBlue. But as it is now, the user would have to type:

Color aliceBlue = new Color { Argb = unchecked((uint)Color.AliceBlue) };

That doesn't quite roll off the tongue, does it?

Aside

Incidentally, another deviation from the pattern I (and CsWin32) are more familiar with is that these struct-value constants are defined as constants of the Apis class. But these color constants are defined in the struct itself. That required a fix to CsWin32. The fix was easy enough, but it does mean that someone asking for the Color struct will immediately get ~148 color constants declared instead of having to request each named color specifically for generation. I'm on the fence on this, but maybe it's best this way so the user can easily select their chosen color from a picklist rather than referring to documentation and then naming the (presumably very few) colors they actually want to use.

[riverar]

gdipluspixelformats.h:

typedef DWORD ARGB;
typedef DWORDLONG ARGB64;

gdipluscolor.h:

class Color
{
// ...
public:
    enum
    {
        AliceBlue            = 0xFFF0F8FF,
        AntiqueWhite         = 0xFFFAEBD7,
        Aqua                 = 0xFF00FFFF,
        Aquamarine           = 0xFF7FFFD4,
        Azure                = 0xFFF0FFFF,
        Beige                = 0xFFF5F5DC,
        Bisque               = 0xFFFFE4C4,
        Black                = 0xFF000000,
        // ...
    }
protected:
    ARGB Argb;
// ...
}

In Microsoft Visual C++ (MSVC), the default underlying type of an unscoped enumeration is int (https://learn.microsoft.com/cpp/build/reference/zc-enumtypes) regardless of enumerator values.

[AArnott]

Thanks for looking into this, @riverar. I see now why the metadata is the way it is. In C++, signed and unsigned integers tend to be more interchangeable than in C# I believe, so I can see why these headers were sufficient.

That said, do you have any concerns with win32metadata adding the necessary overrides to type the constants to match the struct type? That would be enough to enable Color.AliceBlue to be used directly in C#, and I hope it wouldn't upset any other projections since we already have constants defined as structs elsewhere in the metadata. We shouldn't have any native type interop issues from changing signed to unsigned either, since at the interop layer it'll use the struct (or its uint field).

[kennykerr]

Insisting that these enumerator values are signed, because they technically are, when they were clearly indented to be unsigned and MSVC was happy to treat them either way back in the day is not super helpful, essentially forcing every language to inject a cast and requiring things like the AssociatedEnum attribute to try and cover up the friction but that isn't always practical. It would seem to be more helpful if we just faithfully captured the intent of the original API rather than just punting and forcing the issue downstream.

[mikebattista]

Are there any changes outstanding here?

[riverar]

There’s a current tension between projection authors and users, and projection authors and metadata, due to the stance of metadata on preserving ABI and API. The concerns raised by these positions are not well-documented or demonstrated.

For example, we often take firm positions on the handling of unsigned/signed integers in the context of “ABI preservation”. However, there’s no clear evidence that this is ABI significant. On the other hand, it does seem to be important from an “API preservation” perspective. It’s plausible situations involving C++ code generation or maybe static libraries could result in hard-to-identify bugs. I think we might need to improve our messaging on this subject and provide proper documentation.

[AArnott]

If the constants were always used in the Argb field, I guess we could say the constants' signed-ness can be changed. In fact, as constants per say have no ABI at all, IMO the only risk we take by changing the sign on the constants is that some projection somewhere might expose an int API where these colors constants used to fit where making them uint would require a cast to be added to that code for that language. The odds of that seem ... unlikely. So I'd lean toward changing the sign on the constant. And as a matter of policy, ABI is deep and complex and more than I understand, so my 'safe' default would be to not change things that could impact it. But as constants are just values that a compiler will either complain don't fit or will constrain to fit the API/ABI, it seems like a safe policy to change signs/widths of constants but not APIs.