Page MenuHomePhabricator

[llvm-rc] Support not expressions.
ClosedPublic

Authored by jacek on Dec 3 2018, 4:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jacek created this revision.Dec 3 2018, 4:29 PM

LGTM. I presume that you've tested the testcase against rc.exe and that the output is bitexact with it?

I'll leave out the accept stamp for now to hopefully trigger @amccarth or someone else to give it another set of eyes during the next round of US daytime, else I'll stamp and commit it tomorrow.

tools/llvm-rc/ResourceScriptParser.cpp
124 ↗(On Diff #176511)

"is mostly useful for style values"?

mstorsjo added inline comments.Dec 4 2018, 1:06 AM
test/tools/llvm-rc/Inputs/not-expr.rc
13 ↗(On Diff #176511)

Isn't this actually 0x50000022, not just 0x22? The testcase would be easier to grasp if you'd point out what the default style values involved are.

jacek added a comment.Dec 4 2018, 8:48 AM

Thanks for review.

LGTM. I presume that you've tested the testcase against rc.exe and that the output is bitexact with it?

Yes, I verified that the result is identical to rc.exe.

jacek updated this revision to Diff 176795.Dec 5 2018, 4:49 AM
jacek marked an inline comment as done.
mstorsjo added inline comments.Dec 5 2018, 4:55 AM
test/tools/llvm-rc/Inputs/not-expr.rc
13 ↗(On Diff #176795)

0x34 & ~0x15 = 0x20, not 0x22. So this should be "expression evaluates to 0x20". The 0x02 part comes from the default groupbox class style.

jacek marked an inline comment as done.Dec 5 2018, 5:00 AM
jacek added inline comments.
test/tools/llvm-rc/Inputs/not-expr.rc
13 ↗(On Diff #176795)

Oh, right, thanks. I will attach a fixed version.

jacek updated this revision to Diff 176796.Dec 5 2018, 5:00 AM
mstorsjo accepted this revision.Dec 5 2018, 5:13 AM

LGTM, will commit.

This revision is now accepted and ready to land.Dec 5 2018, 5:13 AM
This revision was automatically updated to reflect the committed changes.