This is an archive of the discontinued LLVM Phabricator instance.

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

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



This extends the set of parser's available resources by another one, VERSIONINFO (

Diff Detail


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
483 ↗(On Diff #112217)

No else after return.

495 ↗(On Diff #112217)


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
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
169 ↗(On Diff #113187)

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

173 ↗(On Diff #113187)


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:

  FileVersion 1, 2, 3

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