<div dir="ltr">On Thu, Mar 28, 2013 at 5:08 PM, Nick Kledzik <span dir="ltr"><<a href="mailto:kledzik@apple.com" target="_blank">kledzik@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im"><br>
On Mar 28, 2013, at 12:24 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>> wrote:<br>
<br>
><br>
><br>
> ================<br>
> Comment at: include/lld/Core/TargetInfo.h:76<br>
> @@ +75,3 @@<br>
> +  /// be kept live using this method.<br>
> +  const std::vector<StringRef> deadStripRoots() const {<br>
> +    return _deadStripRoots;<br>
> ----------------<br>
> Missing & here.<br>
</div>Fixed.<br>
<div class="im"><br>
><br>
> ================<br>
> Comment at: include/lld/ReaderWriter/Simple.h:174<br>
> @@ -173,2 +173,3 @@<br>
> public:<br>
> -  SimpleUndefinedAtom(const File &f, StringRef name) : _file(f), _name(name) {}<br>
> +  SimpleUndefinedAtom(const File &f, StringRef name) \<br>
> +    : _file(f)<br>
> ----------------<br>
> No line continuation needed here.<br>
</div>Fixed.  I think I was trying to remove a trailing space and hit the \ key...<br>
<div class="im"><br>
<br>
><br>
> ================<br>
> Comment at: lib/Driver/CoreDriver.cpp:71-74<br>
> @@ +70,6 @@<br>
> +int CoreDriver::main(int argc, const char *argv[]) {<br>
> +  // Standard set up so program fails gracefully.<br>
> +  llvm::sys::PrintStackTraceOnErrorSignal();<br>
> +  llvm::PrettyStackTraceProgram stackPrinter(argc, argv);<br>
> +  llvm::llvm_shutdown_obj shutdown;<br>
> +<br>
> ----------------<br>
> This should be handled by the file that actually defines main. Same for the rest of the drivers.<br>
</div>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()...<br>
</blockquote><div><br></div><div style>I don't think there is any point in providing main.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

<div class="im"><br>
<br>
><br>
> ================<br>
> Comment at: lib/Driver/GnuLdDriver.cpp:1<br>
> @@ +1,2 @@<br>
> +//===- lib/Driver/GNU_ld_Driver.cpp ---------------------------------------===//<br>
> +//<br>
> ----------------<br>
> GnuLdDriver.cpp<br>
</div>Fixed.<br>
<div class="im"><br>
<br>
> ================<br>
> Comment at: lib/ReaderWriter/ELF/ELFTargetInfo.cpp:152<br>
> @@ +151,3 @@<br>
> +      memcpy(x, pathref.data(), pathlen);<br>
> +      this->appendInputFile(StringRef(x,pathlen));<br>
> +      return false;<br>
> ----------------<br>
> No need for this-><br>
</div>Fixed.  Although I don't see anything in the LLVM conventions about not using the this-> to disambiguate.<br>
Also, should memcpy() be std::memcpy()?<br>
<div class="im"><br></div></blockquote><div><br></div><div style>this-> is not used by convention.</div><div style><br></div><div style>Technically memcpy should be std::memcpy, or string.h should be included. I prefer std::, but both are used in llvm and clang.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">
<br>
> ================<br>
> Comment at: lib/ReaderWriter/ELF/Reader.cpp:119<br>
> @@ -119,1 +118,3 @@<br>
> +      return error_code::error_code(ENOEXEC, llvm::system_category());<br>
> +      //llvm_unreachable("not supported format");<br>
>       break;<br>
> ----------------<br>
> Just remove this.<br>
><br>
> ================<br>
> Comment at: lib/ReaderWriter/ELF/Reader.cpp:118<br>
> @@ -117,2 +117,3 @@<br>
>     default:<br>
> -      llvm_unreachable("not supported format");<br>
> +      return error_code::error_code(ENOEXEC, llvm::system_category());<br>
> +      //llvm_unreachable("not supported format");<br>
> ----------------<br>
> This won't work on Windows. Use make_error_code(errc::executable_format_error)<br>
</div>Fixed.<br>
<br>
-Nick<br>
</blockquote></div></div><div class="gmail_extra"><br></div><div class="gmail_extra"><div>- Michael Spencer</div><div><br></div></div></div>