Page MenuHomePhabricator

[llvm-rc] Add VERSIONINFO parsing ability. [6/8]
ClosedPublic

Authored by mnbvmar on Aug 22 2017, 1:45 PM.

Details

Summary

This extends the set of parser's available resources by another one, VERSIONINFO (msdn.microsoft.com/en-us/library/windows/desktop/aa381058(v=vs.85).aspx).

Diff Detail

Repository
rL LLVM

Event Timeline

mnbvmar created this revision.Aug 22 2017, 1:45 PM
mnbvmar edited the summary of this revision. (Show Details)
ecbeckmann added inline comments.Aug 22 2017, 2:46 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
483 ↗(On Diff #112217)

No else after return.

495 ↗(On Diff #112217)

ditto

514 ↗(On Diff #112217)

use early continue pattern, remove else clause

mnbvmar updated this revision to Diff 112446.Aug 23 2017, 2:21 PM
mnbvmar retitled this revision from [llvm-rc] Add VERSIONINFO parsing ability. to [llvm-rc] Add VERSIONINFO parsing ability. [6/8].

Change temp files to pipes in tests; fix readability issues.

mnbvmar marked 3 inline comments as done.Aug 23 2017, 2:22 PM
zturner added inline comments.Aug 25 2017, 1:12 PM
llvm/tools/llvm-rc/ResourceScriptParser.cpp
516–517 ↗(On Diff #112446)

Can you make a RetType::isTypeSupported() function and just call it?

mnbvmar updated this revision to Diff 113187.Aug 29 2017, 6:10 PM

Small style fixes.

mnbvmar marked an inline comment as done.Aug 29 2017, 6:10 PM
rnk added inline comments.Sep 27 2017, 10:53 AM
llvm/tools/llvm-rc/ResourceScriptStmt.cpp
169 ↗(On Diff #113187)

Just .count(Type.upper()) seems more idiomatic.

173 ↗(On Diff #113187)

ditto

llvm/tools/llvm-rc/ResourceScriptStmt.h
387–388 ↗(On Diff #113187)

Generally, you don't need to template over the size of a SmallVector. You can simply take a SmallVectorImpl<T> &, which is the base SmallVectors of any size.

However, for a const reference, it's usually better to accept an ArrayRef<T> Value. That is implicitly constructible from any SmallVector, and passes a pointer and size by value.

389 ↗(On Diff #113187)

Does this need Name.upper()? I'm concerned about things like:

VERSIONINFO
  FileVersion 1, 2, 3
  FILEVERSION 4, 5, 6
BEGIN
END

Should that be an error? Maybe the best way to handle this is to have an enumeration, map from string to enum, maintain an array of all possible fields, and on set, check if the field is already set and report an error if so.

mnbvmar added inline comments.Sep 27 2017, 2:32 PM
llvm/tools/llvm-rc/ResourceScriptStmt.h
389 ↗(On Diff #113187)

The original tool seems not to care about this one and just take the last occurrence of FILEVERSION. Of course, what you suggest is way better, and I think I'm going to do it your way.

mnbvmar updated this revision to Diff 116900.Sep 27 2017, 4:19 PM

Rewritten fixed fields implementation a little bit. Now, we have an enumeration describing fixed field types, a list of field names, and a mapping from names to their types.

Also, added a sanity check determining if the user provided multiple occurrences of the same fixed field.

rnk accepted this revision.Sep 28 2017, 11:09 AM

Looks good!

This revision is now accepted and ready to land.Sep 28 2017, 11:09 AM
This revision was automatically updated to reflect the committed changes.