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.

Sign up for GitHub

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

Jump to bottom

[llvm-rc] add support for MENUEX #67464

Merged
merged 2 commits into from
Oct 6, 2023
Merged

[llvm-rc] add support for MENUEX #67464

merged 2 commits into from
Oct 6, 2023
+293 −11

Conversation

oltolm
Copy link
Contributor

@oltolm oltolm commented Sep 26, 2023

Fixes #39455.

@oltolm oltolm mentioned this pull request Sep 26, 2023
Closed
@mstorsjo mstorsjo added llvm-tools All llvm tools that do not have corresponding tag platform:windows labels Sep 27, 2023
@mstorsjo mstorsjo requested a review from a team September 27, 2023 07:18
@mstorsjo
Copy link
Member

@oltolm Thanks for the contribution! I didn't look at the code yet, but you'll need to add a testcase for this. Have a look at the existing files in llvm-project/llvm/test/tools/llvm-rc for examples of existing tests.

@oltolm
Copy link
Contributor Author

oltolm commented Oct 4, 2023

I added a test.

mstorsjo
mstorsjo reviewed Oct 5, 2023
View reviewed changes
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks! Just a couple minor comments.

llvm/tools/llvm-rc/ResourceScriptStmt.cpp Show resolved Hide resolved
llvm/tools/llvm-rc/ResourceScriptStmt.cpp Show resolved Hide resolved
llvm/tools/llvm-rc/ResourceScriptParser.cpp Outdated Show resolved Hide resolved
llvm/tools/llvm-rc/ResourceScriptParser.cpp Outdated Show resolved Hide resolved
mstorsjo
mstorsjo approved these changes Oct 5, 2023
View reviewed changes
Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. I'll merge it tomorrow or so if there's no further comments.

@mstorsjo mstorsjo merged commit 8b356f4 into llvm:main Oct 6, 2023
3 checks passed
@oltolm oltolm deleted the llvm-rc-menuex branch October 6, 2023 09:09
@waltje
Copy link

waltje commented Oct 7, 2023

Thanks, all!

@stepthomas stepthomas mentioned this pull request Oct 10, 2023
Closed
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 12, 2023
Merged main:8b356f496b18 into amd-gfx:d7a064a92fb5
8bf801a 
Local branch amd-gfx d7a064a Merged main:a06e94cf3b37 into amd-gfx:2cbc551d7f73
Remote branch main 8b356f4 [llvm-rc] add support for MENUEX (llvm#67464)
@mati865 mati865 mentioned this pull request Dec 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mstorsjo mstorsjo mstorsjo approved these changes

Assignees
No one assigned
Labels
llvm-tools All llvm tools that do not have corresponding tag platform:windows
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support MENUEX parsing
3 participants
@oltolm @mstorsjo @waltje