[PATCH] Refactoring of Driver and TargetInfo

[Nick Kledzik]

On Mar 28, 2013, at 12:24 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:

> 
> 
> ================
> Comment at: include/lld/Core/TargetInfo.h:76
> @@ +75,3 @@
> +  /// be kept live using this method.
> +  const std::vector<StringRef> deadStripRoots() const {
> +    return _deadStripRoots;
> ----------------
> Missing & here.
Fixed.

> 
> ================
> Comment at: include/lld/ReaderWriter/Simple.h:174
> @@ -173,2 +173,3 @@
> public:
> -  SimpleUndefinedAtom(const File &f, StringRef name) : _file(f), _name(name) {}
> +  SimpleUndefinedAtom(const File &f, StringRef name) \
> +    : _file(f)
> ----------------
> No line continuation needed here.
Fixed.  I think I was trying to remove a trailing space and hit the \ key...


> 
> ================
> Comment at: lib/Driver/CoreDriver.cpp:71-74
> @@ +70,6 @@
> +int CoreDriver::main(int argc, const char *argv[]) {
> +  // Standard set up so program fails gracefully.
> +  llvm::sys::PrintStackTraceOnErrorSignal();
> +  llvm::PrettyStackTraceProgram stackPrinter(argc, argv);
> +  llvm::llvm_shutdown_obj shutdown;
> +
> ----------------
> This should be handled by the file that actually defines main. Same for the rest of the drivers.
But then there is nothing left in these functions.  My intention was to make these the main() used by the drop in linkers.   If we want the linker tools to call these setup functions, then they could just call the Driver::link() functions too and then we would not need these main() methods.  But I thought it would be simpler for supply that main()...


> 
> ================
> Comment at: lib/Driver/GnuLdDriver.cpp:1
> @@ +1,2 @@
> +//===- lib/Driver/GNU_ld_Driver.cpp ---------------------------------------===//
> +//
> ----------------
> GnuLdDriver.cpp
Fixed.


> ================
> Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:152
> @@ +151,3 @@
> +      memcpy(x, pathref.data(), pathlen);
> +      this->appendInputFile(StringRef(x,pathlen));
> +      return false;
> ----------------
> No need for this->
Fixed.  Although I don't see anything in the LLVM conventions about not using the this-> to disambiguate.  
Also, should memcpy() be std::memcpy()?


> ================
> Comment at: lib/ReaderWriter/ELF/Reader.cpp:119
> @@ -119,1 +118,3 @@
> +      return error_code::error_code(ENOEXEC, llvm::system_category());
> +      //llvm_unreachable("not supported format");
>       break;
> ----------------
> Just remove this.
> 
> ================
> Comment at: lib/ReaderWriter/ELF/Reader.cpp:118
> @@ -117,2 +117,3 @@
>     default:
> -      llvm_unreachable("not supported format");
> +      return error_code::error_code(ENOEXEC, llvm::system_category());
> +      //llvm_unreachable("not supported format");
> ----------------
> This won't work on Windows. Use make_error_code(errc::executable_format_error)
Fixed.

-Nick