This is an archive of the discontinued LLVM Phabricator instance.

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

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



This extends llvm-rc parsing tool by MENU resource ( As for now, MENUEX ( seems unnecessary.

Diff Detail


Event Timeline

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
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.


while (....) {
  if (IsMenuItem && isNextTokenKind(Kind::Identifier)) {
  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.
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.

ecbeckmann edited edge metadata.Aug 22 2017, 2:44 PM

This looks fine now.

rnk added a reviewer: thakis.Aug 23 2017, 9:18 AM
rnk edited edge metadata.Aug 23 2017, 1:13 PM

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

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
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.