This is an archive of the discontinued LLVM Phabricator instance.

Create empty shell of llvm-mt.
ClosedPublic

Authored by ecbeckmann on Jul 12 2017, 4:04 PM.

Details

Summary

This is the first patch towards creating the llvm-mt tool for merging
Windows manifests. This is a reimplementation of mt.exe.

Diff Detail

Repository
rL LLVM

Event Timeline

ecbeckmann created this revision.Jul 12 2017, 4:04 PM
rnk accepted this revision.Jul 14 2017, 11:02 AM

lgtm

This revision is now accepted and ready to land.Jul 14 2017, 11:02 AM
ruiu added inline comments.Jul 14 2017, 12:15 PM
llvm/tools/llvm-mt/Opts.td
3 ↗(On Diff #106335)

By convention, option names are written in lowercase. So unsupported instead of UNSUPPORTED. Apply all other options in this file.

6 ↗(On Diff #106335)

Help text shouldn't end with a full stop.

llvm/tools/llvm-mt/llvm-mt.cpp
48 ↗(On Diff #106335)

This open parenthesis is not at the same indentation depth as the closing one.

66 ↗(On Diff #106335)

It is convenient to write out "\n" here, instead of embedding "\n" as part of Msg.

71 ↗(On Diff #106335)

Please follow the LLVM naming convention.

91 ↗(On Diff #106335)

; -> :

117 ↗(On Diff #106335)

Remove this blank line.

ecbeckmann marked 7 inline comments as done.

Formatting and naming changes to match convention.

ruiu edited edge metadata.Jul 14 2017, 2:02 PM

Looks like this is still the same as the old version. Can you upload again?

In D35333#810069, @ruiu wrote:

Looks like this is still the same as the old version. Can you upload again?

It's different. Perhaps it looks the same because you are looking at the option IDs which I left capitalized, which is actually the convention. However the actual option text that is matched and displayed in the helptext was moved to lower case. All the other changes you mentioned were also performed.

ruiu added a comment.Jul 14 2017, 2:11 PM

Is all uppercase really a convention? I took a quick look at a few .td files in clang, and looks like they write option ids in lowercase.

ruiu added inline comments.Jul 14 2017, 2:13 PM
llvm/tools/llvm-mt/llvm-mt.cpp
48 ↗(On Diff #106335)

Is this done?

66 ↗(On Diff #106335)

It's not done.

71 ↗(On Diff #106335)

argc and argv still don't follow the LLVM naming convention.

ecbeckmann marked an inline comment as done.Jul 14 2017, 6:11 PM
In D35333#810083, @ruiu wrote:

Is all uppercase really a convention? I took a quick look at a few .td files in clang, and looks like they write option ids in lowercase.

hmm it seems the convention is capital first letter, rest lower case

llvm/tools/llvm-mt/llvm-mt.cpp
48 ↗(On Diff #106335)

shoot sorry for some reason git format keeps changing this.

66 ↗(On Diff #106335)

hmmm in most recent revision it should be done

71 ↗(On Diff #106335)

on most recent revision this should be fine

ecbeckmann marked an inline comment as done.Jul 14 2017, 6:17 PM
In D35333#810083, @ruiu wrote:

Is all uppercase really a convention? I took a quick look at a few .td files in clang, and looks like they write option ids in lowercase.

Or it seems there are differing conventions. I'll do lowercase for now then.

corrected case of options

ruiu added inline comments.Jul 14 2017, 6:46 PM
llvm/tools/llvm-mt/llvm-mt.cpp
48 ↗(On Diff #106744)

Align the trailing \.

90 ↗(On Diff #106744)

Error messages should start with lowercase letter.

103 ↗(On Diff #106744)

Ditto

113 ↗(On Diff #106744)

Ditto

66 ↗(On Diff #106335)

Error messages shouldn't end with a full stop. See https://clang.llvm.org/diagnostics.html for example.

71 ↗(On Diff #106335)

Is it? argc and argv should be Argc and Argv.

ecbeckmann marked 4 inline comments as done.

Changed errors and formatting.

ruiu added inline comments.Jul 17 2017, 12:19 PM
llvm/tools/llvm-mt/llvm-mt.cpp
90 ↗(On Diff #106744)

Shouldn't end with a full stop.

66 ↗(On Diff #106335)

Please fix.

71 ↗(On Diff #106335)

Please fix.

ecbeckmann marked an inline comment as done.

remove error full stops

llvm/tools/llvm-mt/llvm-mt.cpp
71 ↗(On Diff #106335)

Hmmm I'm not sure if Argc and Argv are the convention in this case. In all the other "mains" in llvm/tools there is variation between using argc, argc_ and *argv[], argv, *argv_[]. The most common combination is argc and argv.

ecbeckmann marked an inline comment as done.Jul 17 2017, 2:08 PM
ruiu added a comment.Jul 17 2017, 2:20 PM

LGTM

llvm/tools/llvm-mt/llvm-mt.cpp
71 ↗(On Diff #106335)

If that is a convention, that is fine, but I hope you mentioned that instead of just marking this as done.

This revision was automatically updated to reflect the committed changes.