Page MenuHomePhabricator

[llvm-rc] Add DIALOG(EX) parsing ability. [5/8]
ClosedPublic

Authored by mnbvmar on Aug 18 2017, 3:48 PM.

Details

Summary

This extends the set of resources parsed by llvm-rc by DIALOG and DIALOGEX.

Additionally, three optional resource statements specific to these two resources are added: CAPTION, FONT, and STYLE.

Diff Detail

Repository
rL LLVM

Event Timeline

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

remove all else after return in this function, for readability and less redundancy.

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

Code looks good, I just have some testing suggestions.

llvm/test/tools/llvm-rc/parser.test
158 ↗(On Diff #111762)

It would be nice to rewrite these diagnostic tests to use pipes instead of temp files:

; RUN: not llvm-rc ... 2>&1 | \
; RUN:     FileCheck %s --check-prefix=PDIALOGN

Then we have one less thing to number.

179 ↗(On Diff #111762)

Does llvm-rc report a source location along with its error? Checking source location reports with FileCheck has always been quite brittle, so we definitely don't want to check for that in all of these tests, but we should have one test for accurate error locations.

mnbvmar added inline comments.Aug 23 2017, 10:40 AM
llvm/test/tools/llvm-rc/parser.test
179 ↗(On Diff #111762)

As of now, llvm-rc does not have source location information (and it'll be tricky to maintain the accurate location when - some day - the preprocessor comes into play). I think it's a good idea to append this data as soon as we start using Clang preprocessor.

mnbvmar updated this revision to Diff 112398.Aug 23 2017, 10:46 AM

Rewrote tests using temporary files to pipes.

mnbvmar marked an inline comment as done.Aug 23 2017, 10:47 AM
rnk accepted this revision.Aug 23 2017, 1:11 PM

lgtm

This revision is now accepted and ready to land.Aug 23 2017, 1:11 PM
ecbeckmann added inline comments.Aug 23 2017, 1:40 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
255 ↗(On Diff #111762)

Make sure to address this.

mnbvmar updated this revision to Diff 112445.Aug 23 2017, 2:13 PM
mnbvmar retitled this revision from [llvm-rc] Add DIALOG(EX) parsing ability. to [llvm-rc] Add DIALOG(EX) parsing ability. [5/8].

Rebased the code; addressed ecbeckmann's comment.

This revision was automatically updated to reflect the committed changes.