[llvm-rc] Add MENU parsing ability. [4/8]
ClosedPublic

Authored by mnbvmar on Aug 18 2017, 2:18 PM.

Details

Summary

This extends llvm-rc parsing tool by MENU resource (msdn.microsoft.com/en-us/library/windows/desktop/aa381025(v=vs.85).aspx). As for now, MENUEX (msdn.microsoft.com/en-us/library/windows/desktop/aa381023(v=vs.85).aspx) seems unnecessary.

Diff Detail

Repository
rL LLVM
mnbvmar updated this revision to Diff 111751.Aug 18 2017, 2:45 PM

Small bugfixes (compilation problems on gcc and missing newlines at the end of the files).

ecbeckmann added inline comments.Aug 18 2017, 4:25 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
316 ↗(On Diff #111751)

I've never seen this before. Is there a reason to using a comma instead of just putting as a separate statement?

320 ↗(On Diff #111751)

do the early continue for better readability, and remove the else block.

i.e.

while (....) {
......
  if (IsMenuItem && isNextTokenKind(Kind::Identifier)) {
  .....
  continue;
  }
  ASSIGN_OR_RETURN(CaptionResult, readString());
  .......
}
323 ↗(On Diff #111751)

doesn't ASSIGN_OR_RETURN go here?

344 ↗(On Diff #111751)

can use same early continue pattern here.

393 ↗(On Diff #111751)

Sorry this isn't part of this revision, but this should have a better name, like just "createError()"

mnbvmar updated this revision to Diff 112223.Aug 22 2017, 2:05 PM

Small fix-ups for the problems mentioned above.

mnbvmar marked 4 inline comments as done.Aug 22 2017, 2:07 PM
mnbvmar added inline comments.
llvm/tools/llvm-rc/ResourceScriptParser.cpp
393 ↗(On Diff #111751)

OK, might be. As this name is scattered around the file, I suggest that I refactor it as soon as all the parser parts are accepted.

This looks fine now.

rnk added a comment.Aug 23 2017, 1:13 PM

Can you rebase this and the other patches and I'll take another look?

llvm/test/tools/llvm-rc/parser.test
120 ↗(On Diff #112223)

The same comment re: pipes applies here.

mnbvmar updated this revision to Diff 112444.Aug 23 2017, 2:12 PM
mnbvmar retitled this revision from [llvm-rc] Add MENU parsing ability. to [llvm-rc] Add MENU parsing ability. [4/8].

Use pipes instead of temporary files.

rnk added inline comments.Aug 23 2017, 5:24 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
343 ↗(On Diff #112444)

ArrayRef will help here too.

zturner accepted this revision.Aug 25 2017, 1:09 PM
This revision is now accepted and ready to land.Aug 25 2017, 1:09 PM
This revision was automatically updated to reflect the committed changes.