This is an archive of the discontinued LLVM Phabricator instance.

[llvm-rc] Add ACCELERATORS parsing ability. [3/8]
ClosedPublic

Authored by mnbvmar on Aug 18 2017, 1:41 PM.

Details

Summary

This improves the current llvm-rc parser by the ability of parsing ACCELERATORS statement.

Diff Detail

Repository
rL LLVM

Event Timeline

mnbvmar created this revision.Aug 18 2017, 1:41 PM
mnbvmar created this object with visibility "mnbvmar (Marek Sokołowski)".
mnbvmar created this object with edit policy "mnbvmar (Marek Sokołowski)".
mnbvmar edited the summary of this revision. (Show Details)
mnbvmar added reviewers: zturner, rnk, ecbeckmann.
mnbvmar changed the visibility from "mnbvmar (Marek Sokołowski)" to "Public (No Login Required)".
mnbvmar changed the edit policy from "mnbvmar (Marek Sokołowski)" to "All Users".
ecbeckmann added inline comments.Aug 18 2017, 2:21 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
127 ↗(On Diff #111734)

no else after return

ecbeckmann added inline comments.Aug 18 2017, 2:28 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
127 ↗(On Diff #111734)

Actually it looks like IntOrString has a constructor taking a Token, but currently isn't used anywhere and has no implementation. Is this intended and if so you could use this constructor instead of having if block?

mnbvmar added inline comments.Aug 18 2017, 2:40 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
127 ↗(On Diff #111734)

No, this was not intended. I remember writing it to support this method and the one below (readTypeOrName). I'll fix it in a moment.

mnbvmar updated this revision to Diff 111750.Aug 18 2017, 2:42 PM

Uses IntOrString(Token) instead of if-s.

mnbvmar marked 3 inline comments as done.Aug 18 2017, 2:42 PM
mnbvmar updated this revision to Diff 112442.Aug 23 2017, 2:11 PM
mnbvmar retitled this revision from [llvm-rc] Add ACCELERATORS parsing ability. to [llvm-rc] Add ACCELERATORS parsing ability. [3/8].

Use pipes instead of temporary files.

rnk added inline comments.Aug 23 2017, 5:21 PM
llvm/tools/llvm-rc/ResourceScriptParser.h
106 ↗(On Diff #112442)

I'd recommend ArrayRef<StringRef> here. Then callers of parseFlags don't need to pass NumFlags separately.

zturner edited edge metadata.Aug 25 2017, 1:05 PM

Just the below style changes along with rnk@'s suggestion about ArrayRef<StringRef> and this is good.

llvm/tools/llvm-rc/ResourceScriptParser.cpp
206–214 ↗(On Diff #112442)
if (!FlagResult->equals_lower(FlagDesc[FlagId]))
  continue;
217–219 ↗(On Diff #112442)

How about

std::string ExpectedList = llvm::join(FlagDesc, "/");
mnbvmar updated this revision to Diff 112961.Aug 28 2017, 1:49 PM

Small style fixes.

mnbvmar marked 3 inline comments as done.Aug 28 2017, 1:50 PM
zturner accepted this revision.Aug 28 2017, 1:56 PM
This revision is now accepted and ready to land.Aug 28 2017, 1:56 PM
This revision was automatically updated to reflect the committed changes.