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

String constants need attribute indicating original encoding #1008

Closed
kennykerr opened this issue Jul 25, 2022 · 9 comments · Fixed by #1309
Closed

String constants need attribute indicating original encoding #1008

kennykerr opened this issue Jul 25, 2022 · 9 comments · Fixed by #1309
Assignees
Labels
enhancement New feature or request rust Critical for Rust adoption

Comments

@kennykerr
Copy link
Contributor

Constants like BCRYPT_3DES_ALGORITHM need to indicate whether they were originally ANSI strings like "3DES" or wide strings like L"3DES" because statically compiled languages need to bind them as compile time constants.

@riverar
Copy link
Collaborator

riverar commented Jul 28, 2022

We have NativeTypeName, NativeTypedef, NativeArrayInfo, NativeInheritance, and other similar attributes, so perhaps something like this is in order?

// ─── wincrypt.h

// #define CERT_PIN_RULES_CTL_FILENAME_A               "pinrules.stl"
[NativeEncoding("ansi")]
public const string CERT_PIN_RULES_CTL_FILENAME_A = "pinrules.stl";

// #define CERT_PIN_RULES_CTL_FILENAME                 L"pinrules.stl"
[NativeEncoding("utf16")]
public const string CERT_PIN_RULES_CTL_FILENAME = "pinrules.stl";

// ─── bcrypt.h

// #define BCRYPT_DH_PARAMETERS   L"DHParameters"
[NativeEncoding("utf16")]
public const string BCRYPT_DH_PARAMETERS = "DHParameters";

Or would folks prefer a Windows.Win32.Interop.Encoding enumeration over the string?

@mikebattista
Copy link
Collaborator

Would need to update the ConstantsScraper to differentiate the various string types and add the attribute.

@monax3
Copy link

monax3 commented Oct 8, 2022

This would be very helpful, as otherwise these strings are left in a frustrating to deal with state instead of being directly usable as is their intent

@kennykerr
Copy link
Contributor Author

Yep, we'd really appreciate a fix for this issue.

WaitingGIF

@mikebattista
Copy link
Collaborator

Couple questions from looking into this:

  • How do we handle TEXT macros?
  • Do we attribute every string or assume lack of an attribute indicates a default like utf-16 and we only attribute strings we know are always ansi?

@kennykerr
Copy link
Contributor Author

How do we handle TEXT macros?

That's a conditional macro used for APIs that have both an A and a W version where the W version is preferred so I suggest those are always compiled with UNICODE and thus land up as UTF16 strings.

Do we attribute every string or assume lack of an attribute indicates a default like utf-16 and we only attribute strings we know are always ansi?

The headers don't have attributes. They're either ansi or wide string literals so I wouldn't imagine there would be any ambiguity.

@mikebattista
Copy link
Collaborator

That's a conditional macro used for APIs that have both an A and a W version where the W version is preferred so I suggest those are always compiled with UNICODE and thus land up as UTF16 strings.

Sounds good.

The headers don't have attributes. They're either ansi or wide string literals so I wouldn't imagine there would be any ambiguity.

I meant in the metadata. Do we want to put an explicit attribute on every string in the metadata? Or assume utf-16 for example and only add an attribute for ansi?

@kennykerr
Copy link
Contributor Author

Sure, that sounds good.

mikebattista added a commit that referenced this issue Oct 11, 2022
mikebattista added a commit that referenced this issue Oct 11, 2022
* Fixed #1008.

* Updated the baseline.

* Merged main and resolved conflicts.

* Fixed bad merge.

* Removed .gitattributes union merge from #1246.

This did not solve the issues with web PRs and is not behaving as expected for local merges with git.exe.
@kennykerr
Copy link
Contributor Author

Thanks Mike, this is now plumbed through to Rust: microsoft/windows-rs#2101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request rust Critical for Rust adoption
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants