Skip to content

[llvm-rc] Add support for multiplication and division in expressions #143373

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

Merged
merged 1 commit into from
Jun 10, 2025

Conversation

mstorsjo
Copy link
Member

@mstorsjo mstorsjo commented Jun 9, 2025

This is supported by GNU windres. MS rc.exe does accept these expressions, but doesn't evalulate them correctly, it only returns the left hand side.

This fixes one aspect of #143157.

@cjacek
Copy link
Contributor

cjacek commented Jun 9, 2025

I was wondering if we should handle operator precedence correctly, like in 1 + 2 * 3. With this PR, we currently evaluate addition first, resulting in 9. GNU windres seems to handle precedence properly and gives 7. MSVC rc.exe is odd, it appears to ignore multiplication and division entirely, just outputting 3.

I'm not sure what the best approach is here, what do you think?

@mstorsjo
Copy link
Member Author

mstorsjo commented Jun 9, 2025

I was wondering if we should handle operator precedence correctly, like in 1 + 2 * 3. With this PR, we currently evaluate addition first, resulting in 9. GNU windres seems to handle precedence properly and gives 7. MSVC rc.exe is odd, it appears to ignore multiplication and division entirely, just outputting 3.

I'm not sure what the best approach is here, what do you think?

Oh, that's indeed a good point, thanks for catching it!

That's weird if rc.exe does accept them but actually doesn't act on them. So perhaps they're not really a thing in canonical MS rc files - I guess that's why they're not implemented so far.

The case where I ran into this, https://github.com/DerellLicht/winwiz/blob/ed4134c44c22acc3746228eec34538d1e857dd6c/winwiz.rc#L250-L253, has parentheses so it should be unambiguous anyway (I think, or does it not honor them correctly either?), but if we do implement them I do think we should get the precedence right as well.

@mstorsjo mstorsjo force-pushed the llvm-rc-expr-mult-div branch from 8fc8081 to 217d30b Compare June 10, 2025 07:55
@mstorsjo
Copy link
Member Author

Thanks, now it should handle operator precedence correctly.

(It may have been possible to handle this by sticking to two levels of expression evaluation, but for clarity, I made a second level inbetween the preexisting low precendence binary operators, and the existing unary operators.)

Copy link
Contributor

@cjacek cjacek left a comment

Choose a reason for hiding this comment

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

The code looks good to me, but please update the parser summary comment above RCParser::readInt.

This is supported by GNU windres. MS rc.exe does accept these expressions,
but doesn't evalulate them correctly, it only returns the left hand side.

This fixes one aspect of
llvm#143157.
@mstorsjo mstorsjo force-pushed the llvm-rc-expr-mult-div branch from 217d30b to d04c540 Compare June 10, 2025 11:27
@mstorsjo
Copy link
Member Author

The code looks good to me, but please update the parser summary comment above RCParser::readInt.

Thanks for catching this detail as well! Updated now, will push later today.

@mstorsjo mstorsjo merged commit d7282c5 into llvm:main Jun 10, 2025
7 checks passed
@mstorsjo mstorsjo deleted the llvm-rc-expr-mult-div branch June 10, 2025 20:34
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
…lvm#143373)

This is supported by GNU windres. MS rc.exe does accept these
expressions, but doesn't evalulate them correctly, it only returns the
left hand side.

This fixes one aspect of
llvm#143157.
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
…lvm#143373)

This is supported by GNU windres. MS rc.exe does accept these
expressions, but doesn't evalulate them correctly, it only returns the
left hand side.

This fixes one aspect of
llvm#143157.
akuhlens pushed a commit to akuhlens/llvm-project that referenced this pull request Jun 24, 2025
…lvm#143373)

This is supported by GNU windres. MS rc.exe does accept these
expressions, but doesn't evalulate them correctly, it only returns the
left hand side.

This fixes one aspect of
llvm#143157.
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.

2 participants