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

Fix VERSIONINFO for llvm-rc #206

Merged
merged 2 commits into from
Feb 22, 2023
Merged

Fix VERSIONINFO for llvm-rc #206

merged 2 commits into from
Feb 22, 2023

Conversation

yeah-its-gloria
Copy link
Contributor

Closes #155. Also fixes a printf format warning.

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! I have one minor comment - not a must-fix but a nice-to-have.

loader/windows/icd_windows.c Outdated Show resolved Hide resolved
@Kerilk
Copy link
Contributor

Kerilk commented Feb 15, 2023

Thanks for this contribution! I have a small question:

Disclaimer: I am no expert in Windows programming, so I may be saying something irrelevant here.

When looking at:
https://learn.microsoft.com/en-us/windows/win32/menurc/stringfileinfo-block
It seems that "040904E4" means that the values should be in U.S. English and in Multilingual character set (0x04E4 == 1252).
Thus shouldn't all the strings be prefixed by L here? Are we mixing character sets? Tagging @jenatali .

@Kerilk Kerilk self-requested a review February 15, 2023 21:37
@yeah-its-gloria
Copy link
Contributor Author

yeah-its-gloria commented Feb 16, 2023

By default, all strings are treated as ASCII in either resource compiler, however MSVC's RC automatically turns strings that use codepoints beyond that set into UTF-16, which llvm-rc doesn't.

@bashbaug
Copy link
Contributor

I filed #209 to track the question about string prefixes. I'm going to merge this now though, since this is definitely an improvement over what we have. Thank you for your contribution!

@bashbaug bashbaug merged commit b0f1c3c into KhronosGroup:main Feb 22, 2023
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.

Can't compile OpenCL.rc on Windows using llvm-rc
4 participants