[PATCH] Refactoring of Driver and TargetInfo

[Michael Spencer]

On Thu, Mar 28, 2013 at 5:08 PM, Nick Kledzik <kledzik at apple.com> wrote:

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

I don't think there is any point in providing 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()?
>
>
this-> is not used by convention.

Technically memcpy should be std::memcpy, or string.h should be included. I
prefer std::, but both are used in llvm and clang.


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

- Michael Spencer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130328/669d7667/attachment.html>