Make empty shell of new cvtres tool.
ClosedPublic

Authored by ecbeckmann on Fri, Apr 14, 2:12 PM.

Details

Summary

This patch introduces a currently unimplemented cvtres tool. The only
thing the tool does is take some command line flags and print them out
if they match the expected ones.

Diff Detail

Repository
rL LLVM
ecbeckmann created this revision.Fri, Apr 14, 2:12 PM
ecbeckmann updated this revision to Diff 95515.Mon, Apr 17, 5:43 PM

Actually got it to compile and run properly.

ecbeckmann added a reviewer: thakis.

I'm out today. Since lld will be a client of the library code of this tool, Rui should probably take a look if he's around this week. I'd also already add a test with just a RUN line for running the tool without parameters, just to have test coverage from day one (makes it easier to add tests in the future once the tool actually does something).

ecbeckmann updated this revision to Diff 95639.Tue, Apr 18, 2:52 PM

Added one line test.

Can you make the test run llvm-cvtres.exe /? and then have a FileCheck line that matches the first line of help text?

llvm/tools/llvm-cvtres/llvm-cvtres.cpp
57 ↗(On Diff #95639)

Can you add these lines to the top?

sys::PrintStackTraceOnErrorSignal(argv_[0]);
PrettyStackTraceProgram X(argc_, argv_);

ExitOnErr.setBanner("llvm-cvtres: ");

SmallVector<const char *, 256> argv;
SpecificBumpPtrAllocator<char> ArgAllocator;
ExitOnErr(errorCodeToError(sys::Process::GetArgumentVector(
    argv, makeArrayRef(argv_, argc_), ArgAllocator)));

llvm_shutdown_obj Y; // Call llvm_shutdown() on exit.

This is basically just boiler plate stuff that makes it so if the tool crashes you get a nice stack trace and that everything terminates cleanly on both a clean and unclean shutdown.

ecbeckmann updated this revision to Diff 95674.Tue, Apr 18, 6:45 PM

Added help text, and a basic check for it.

ecbeckmann marked an inline comment as done.Tue, Apr 18, 6:46 PM
ruiu added inline comments.Wed, Apr 19, 4:21 AM
llvm/tools/llvm-cvtres/Opts.td
12 ↗(On Diff #95674)

Please fix.

llvm/tools/llvm-cvtres/llvm-cvtres.cpp
60 ↗(On Diff #95674)

Ah, so this command is a stub. It is fine to submit a command that can do a minimal thing, but it feels to me that this is too small. Can you add a feature so that you can say that this command satisfies minimal user needs?

llvm/tools/llvm-cvtres/llvm-cvtres.h
1 ↗(On Diff #95674)

This file doesn't seem to make sense. Header files are not mandatory. If you don't have anything to put into a header, you don't need to create it.

Did you run clang-format on the CL? (It looks clang-formatted to me, just want to make sure)

llvm/tools/llvm-cvtres/llvm-cvtres.cpp
60 ↗(On Diff #95674)

Mostly agree with Nico's comments here. There is nothing it can really do that is more minimal than the full functionality of the program. At least with the stub we don't have to worry about reviewing all this boilerplate once the real code goes in.

75 ↗(On Diff #95674)

You have using namespace llvm at the top, so no need for the explicit qualifiers.

76 ↗(On Diff #95674)

s/inputArgs/InputArgs. Variable names are always capitalized, including the first letter.

78–97 ↗(On Diff #95674)

I would remove all these lines (leave the Help line though), as you're basically just verifying that the lib/Option works as advertised, which doesn't really buy us anything since we have a separate suite of tests specifically for lib/Object itself. All of these will go away once the individual options are implemented, so it can't even really be considered stub code, but rather throwaway code.

ecbeckmann marked 4 inline comments as done.

small fixes

thakis accepted this revision.Wed, Apr 19, 1:08 PM

Looks good to me.

This revision is now accepted and ready to land.Wed, Apr 19, 1:08 PM
zturner accepted this revision.Wed, Apr 19, 1:11 PM

lgtm, I can commit it for you later today.

thakis added inline comments.Wed, Apr 19, 1:17 PM
llvm/tools/llvm-cvtres/Opts.td
6 ↗(On Diff #95779)

I tried to leave a comment saying that http://llvm-cs.pcc.me.uk/tools/clang/tools/clang-format/ClangFormat.cpp#1 doesn't need a .td file for flag handling, and that maybe it's overkill here too (but it probably doesn't matter much). Looks like it didn't get sent with my last message, so trying again...

ecbeckmann added inline comments.Wed, Apr 19, 2:17 PM
llvm/tools/llvm-cvtres/Opts.td
6 ↗(On Diff #95779)

That looks like it's because ClangFormat.cpp uses the CommandLine library instead of the Option library. We had to use the Option library in order to support flags given with slashes.

This revision was automatically updated to reflect the committed changes.