Page MenuHomePhabricator

[llvm-rc] Support EXSTYLE statement.
ClosedPublic

Authored by jacek on Nov 28 2018, 1:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

jacek created this revision.Nov 28 2018, 1:19 PM

LGTM although you may want to get a review from someone who's more familiar with llvm-rc.

tools/llvm-rc/ResourceScriptStmt.h
872 ↗(On Diff #175753)

Nit: I'd consider naming it just ExStyle to match the name of the statement precisely.

Looks great in general, but I think I agree that having it named ExStyle would be less prone to confusion.

tools/llvm-rc/ResourceVisitor.h
53 ↗(On Diff #175753)

These were alpabetically ordered before. The same goes for a few other places in the patch as well.

jacek updated this revision to Diff 175846.Nov 29 2018, 3:42 AM

Thanks for reviews, I addressed comments.

FWIW, I used ExtStyle instead of ExStyle in previous version because that's what's used in a few places in sources already, but I agree that ExStyle fits better.

mstorsjo accepted this revision.Nov 29 2018, 4:03 AM

LGTM

However, I can't commit it as is, as the diff for test/tools/llvm-rc/Inputs/tag-dialog-headers.rc is missing, as it's treated as binary. I'm not sure exactly why we have that there, but you can e.g. remove the relevant line from .gitattributes before diffing, to get that file included in the diff.

tools/llvm-rc/ResourceVisitor.h
30 ↗(On Diff #175846)

This one is still unordered, but I could change that before committing.

This revision is now accepted and ready to land.Nov 29 2018, 4:03 AM
jacek updated this revision to Diff 175850.Nov 29 2018, 4:11 AM
This revision was automatically updated to reflect the committed changes.