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.
Details
Diff Detail
- Build Status
Buildable 5567 Build 5567: arc lint + arc unit
Event Timeline
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).
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 | ||
---|---|---|
58 | 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. |
llvm/tools/llvm-cvtres/Opts.td | ||
---|---|---|
9 | Please fix. | |
llvm/tools/llvm-cvtres/llvm-cvtres.cpp | ||
61 | 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 | ||
2 | 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 | ||
---|---|---|
61 | 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. | |
76 | You have using namespace llvm at the top, so no need for the explicit qualifiers. | |
77 | s/inputArgs/InputArgs. Variable names are always capitalized, including the first letter. | |
79–98 | 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. |
llvm/tools/llvm-cvtres/Opts.td | ||
---|---|---|
7 | 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... |
llvm/tools/llvm-cvtres/Opts.td | ||
---|---|---|
7 | 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. |
Standalone build has been broken?
I installed llvm svn r. 301591, it does not ship with llvm-cvtres. This state has been reported by clang, which fails to pass configure...
-- The C compiler identification is GNU 5.4.0 -- The CXX compiler identification is GNU 5.4.0 -- Check for working C compiler: /tmp/pkgsrc-tmp/wip/clang-netbsd/work/.cwrapper/bin/gcc -- Check for working C compiler: /tmp/pkgsrc-tmp/wip/clang-netbsd/work/.cwrapper/bin/gcc -- works -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Detecting C compile features -- Check for working CXX compiler: /tmp/pkgsrc-tmp/wip/clang-netbsd/work/.cwrapper/bin/g++ -- Check for working CXX compiler: /tmp/pkgsrc-tmp/wip/clang-netbsd/work/.cwrapper/bin/g++ -- works -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Detecting CXX compile features -- Detecting CXX compile features - done -- Found LLVM_CONFIG as /usr/pkg/bin/llvm-config CMake Error at /usr/pkg/lib/cmake/llvm/LLVMExports.cmake:614 (message): The imported target "llvm-cvtres" references the file "/usr/pkg/bin/llvm-cvtres" but this file does not exist. Possible reasons include: * The file was deleted, renamed, or moved to another location. * An install or uninstall procedure did not complete successfully. * The installation package was faulty and contained "/usr/pkg/lib/cmake/llvm/LLVMExports.cmake" but not all the files it references. Call Stack (most recent call first): /usr/pkg/lib/cmake/llvm/LLVMConfig.cmake:138 (include) CMakeLists.txt:70 (include) -- Configuring incomplete, errors occurred! See also "/tmp/pkgsrc-tmp/wip/clang-netbsd/work/build/CMakeFiles/CMakeOutput.log". *** Error code 1 Stop. make[1]: stopped in /usr/pkgsrc/wip/clang-netbsd *** Error code 1 Stop. make: stopped in /usr/pkgsrc/wip/clang-netbsd
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...