[PATCH] D54379: Add Hurd toolchain support to Clang

[Samuel Thibault via Phabricator via cfe-commits]

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