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

scraper/enums: Define enum for Windows.Win32.Graphics.Hlsl.DXC_CP #474

Merged
merged 4 commits into from
May 19, 2021

Conversation

MarijnS95
Copy link
Contributor

@MarijnS95 MarijnS95 commented May 13, 2021

From the discussion in #450 (comment)

This results in the following enum:

// Windows.Win32.Graphics.Hlsl.DXC_CP
public enum DXC_CP : uint
{
	DXC_CP_UTF8 = 65001u,
	DXC_CP_UTF16 = 1200u,
	DXC_CP_ACP = 0u
}

With the following replacements:

Windows.Win32.Graphics.Hlsl.IDxcBlobEncoding.GetEncoding : pCodePage...UInt32* => DXC_CP*
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobFromFile : codePage...UInt32* => DXC_CP*
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingFromPinned : codePage...UInt32 => DXC_CP
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingOnHeapCopy : codePage...UInt32 => DXC_CP
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingOnMalloc : codePage...UInt32 => DXC_CP
Windows.Win32.Graphics.Hlsl.DXC_CP not found in 1st winmd
Old comment when this was done for all of `CP_`

This results in the following enum:

// Windows.Win32.Globalization.CODEPAGE
public enum CODEPAGE : uint
{
    CP_INSTALLED = 1u,
    CP_SUPPORTED = 2u,
    CP_ACP = 0u,
    CP_OEMCP = 1u,
    CP_MACCP = 2u,
    CP_THREAD_ACP = 3u,
    CP_SYMBOL = 42u,
    CP_UTF7 = 65000u,
    CP_UTF8 = 65001u
}

With the following replacements:

Windows.Win32.Graphics.Hlsl.IDxcBlobEncoding.GetEncoding : pCodePage...UInt32* => CODEPAGE*
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobFromFile : codePage...UInt32* => CODEPAGE*
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingFromPinned : codePage...UInt32 => CODEPAGE
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingOnHeapCopy : codePage...UInt32 => CODEPAGE
Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingOnMalloc : codePage...UInt32 => CODEPAGE
Windows.Win32.Globalization.CPINFOEXA.CodePage...UInt32 => CODEPAGE
Windows.Win32.Globalization.CPINFOEXW.CodePage...UInt32 => CODEPAGE
Windows.Win32.Globalization.CODEPAGE not found in 1st winmd
Windows.Win32.Globalization.Apis.GetCPInfo : CodePage...UInt32 => CODEPAGE
Windows.Win32.Globalization.Apis.GetCPInfoExA : CodePage...UInt32 => CODEPAGE
Windows.Win32.Globalization.Apis.GetCPInfoExW : CodePage...UInt32 => CODEPAGE

As mentioned in #450 (comment) "filter": "CP_" picks up two unrelated constants already defined in another enum:

Duplicate constants/enum names detected:
CP_INSTALLED
  Windows.Win32.Globalization.CODEPAGE
  Windows.Win32.Globalization.ENUM_SYSTEM_CODE_PAGES_FLAGS
CP_SUPPORTED
  Windows.Win32.Globalization.CODEPAGE
  Windows.Win32.Globalization.ENUM_SYSTEM_CODE_PAGES_FLAGS

TODO

  • Is the name CODEPAGE okay?
  • Any more functions/types that need to be annotated?

Note on DXC

@mikebattista
Copy link
Contributor

New contributing guidelines are up at https://github.com/microsoft/win32metadata/blob/master/CONTRIBUTING.md.

I don't think the CP_INSTALLED and CP_SUPPORTED should be in the same enum as the others. As the duplicate shows, it's used by the enum API.

Also the GetCPInfo APIs take an open set of values it seems from the docs, not just the ones from the enum. @sotteson1 is an enum the proper choice here for an open set?

@MarijnS95 MarijnS95 force-pushed the codepage branch 2 times, most recently from c301eac to 1fd89ba Compare May 14, 2021 06:41
@MarijnS95
Copy link
Contributor Author

New contributing guidelines are up at https://github.com/microsoft/win32metadata/blob/master/CONTRIBUTING.md.

That's nice and useful, thanks!

I don't think the CP_INSTALLED and CP_SUPPORTED should be in the same enum as the others. As the duplicate shows, it's used by the enum API.

Correct, these are explicitly listed in enums.json under ENUM_SYSTEM_CODE_PAGES_FLAGS. Asked above if there's an easy way to omit them from autoPopulate's filter?

Also the GetCPInfo APIs take an open set of values it seems from the docs, not just the ones from the enum. @sotteson1 is an enum the proper choice here for an open set?

You're right, the docs mention a lot more values than are available in constants, perhaps it's not a good idea to define this for CODEPAGE then. OTOH DXC explicitly supports three pages and nothing more, so we could have a DXC_CP enum here then.

The values are not in the SDK header yet but should hopefully be in some future release, does it make sense to list the members explicitly now and have "filter": "DXC_CP_" already in place, so that it hopefully errors out on a conflict when the manual members are ready to be removed again?

Should I order the members by value, for cleanliness? Copypasted them from the header but that seems to be arbitrary ordering anyway.

@MarijnS95 MarijnS95 changed the title scraper/enums: Define enum for Windows.Win32.Globalization.CODEPAGE scraper/enums: Define enum for Windows.Win32.Graphics.Hlsl.DXC_CP May 14, 2021
@mikebattista
Copy link
Contributor

You're right, the docs mention a lot more values than are available in constants, perhaps it's not a good idea to define this for CODEPAGE then. OTOH DXC explicitly supports three pages and nothing more, so we could have a DXC_CP enum here then.

I prefer the DXC_CP approach with constrained values just for DXC.

The values are not in the SDK header yet but should hopefully be in some future release, does it make sense to list the members explicitly now and have "filter": "DXC_CP_" already in place, so that it hopefully errors out on a conflict when the manual members are ready to be removed again?

I would just manually define them for now. We can make future changes to the metadata when the time comes if necessary.

Should I order the members by value, for cleanliness? Copypasted them from the header but that seems to be arbitrary ordering anyway.

I would order them by value (0 first) so the enum members are ordered by value as you'd expect.

This results in the following enum:

    // Windows.Win32.Graphics.Hlsl.DXC_CP
    public enum DXC_CP : uint
    {
        DXC_CP_ACP = 0u
        DXC_CP_UTF16 = 1200u,
        DXC_CP_UTF8 = 65001u,
    }

With the following replacements:

    Windows.Win32.Graphics.Hlsl.IDxcBlobEncoding.GetEncoding : pCodePage...UInt32* => DXC_CP*
    Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobFromFile : codePage...UInt32* => DXC_CP*
    Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingFromPinned : codePage...UInt32 => DXC_CP
    Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingOnHeapCopy : codePage...UInt32 => DXC_CP
    Windows.Win32.Graphics.Hlsl.IDxcLibrary.CreateBlobWithEncodingOnMalloc : codePage...UInt32 => DXC_CP
    Windows.Win32.Graphics.Hlsl.DXC_CP not found in 1st winmd

In future DXC headers a new `IDxcUtils` class (taking the place of
`IDxcLibrary`) is introduced which also takes `DXC_CP_` enum values in
its `codePage` parameters [1].  It is listed here preliminarily
anticipating it being added to the headers in a newer SDK.

[1]: https://github.com/microsoft/DirectXShaderCompiler/blob/aa4f5176ebba010a860a214d73f33bdc5b5042ce/include/dxc/dxcapi.h#L356-L390
@MarijnS95
Copy link
Contributor Author

I would just manually define them for now. We can make future changes to the metadata when the time comes if necessary.

They're manually defined now, but I feel like keeping "filter": "DXC_CP_" so that this extra bit of automation is not forgotten about when the next SDK (or perhaps the one after) includes these.

I would order them by value (0 first) so the enum members are ordered by value as you'd expect.

👍 They're in the right order now.

I prefer the DXC_CP approach with constrained values just for DXC.

Cool, then we're on the same page, and this PR is ready for review and merging :)

@mikebattista mikebattista merged commit 087e855 into microsoft:master May 19, 2021
@mikebattista
Copy link
Contributor

Merged. Thanks for the contribution.

@MarijnS95 MarijnS95 deleted the codepage branch May 19, 2021 06:38
@teoxoy
Copy link

teoxoy commented Aug 30, 2024

@MarijnS95 I think DxcBuffer.Encoding should also be DXC_CP instead of u32 but I'm not 100% sure. Thoughts?

@MarijnS95
Copy link
Contributor Author

@teoxoy for completeness, beb18b9 added DxcBuffer exactly 5 months after this PR was opened :)

I think you are right, at least the // Use Encoding = 0 for non-text bytes, ANSI text, or unknown with BOM. description for ANSI and UTF BOM autodetection matches the comment on DXC_CP_ACP.

More DXC annotations appear to be missing; I cobbled together some in #1989 but without any codegen/testing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants