sthibaul marked 13 inline comments as done. sthibaul added a comment. I'll update the diff according to the comments.
================ Comment at: lib/Driver/ToolChains/Hurd.cpp:74 + + // Similar to the logic for GCC above, if we currently running Clang inside + // of the requested system root, add its parent library paths to ---------------- aaron.ballman wrote: > What GCC logic above? > > we currently -> we are currently That was from Linux.cpp. I have not yet included the gcc pieces above, so the comment doesn't make sense yet indeed. ================ Comment at: lib/Driver/ToolChains/Hurd.cpp:120 + + llvm_unreachable("unsupported architecture"); +} ---------------- aaron.ballman wrote: > This doesn't look unreachable to me? For now it is, we only have x86 architecture for the Hurd. This is like Linux::getDynamicLinker which uses llvm_unreachable in the default case. ================ Comment at: lib/Driver/ToolChains/Hurd.h:31-33 + virtual std::string computeSysRoot() const; + + virtual std::string getDynamicLinker(const llvm::opt::ArgList &Args) const; ---------------- aaron.ballman wrote: > Why are these virtual functions? > > `getDynamicLinker()` is implemented but never called in this patch -- is it > needed? I don't know the rationale, I am just reusing the same principle as in Linux.{h,cpp}. getDynamicLinker is used by Gnu.cpp's ConstructJob. ================ Comment at: lib/Driver/ToolChains/Hurd.h:35 + + std::vector<std::string> ExtraOpts; + ---------------- aaron.ballman wrote: > This appears to be entirely unused. Again, it is used by Gnu.cpp's ConstructJob Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54379/new/ https://reviews.llvm.org/D54379 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits