[PATCH] D54379: Add Hurd toolchain support to Clang

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I don't know enough about hurd to review those portions of it, but I have some general comments on the patch.



================
Comment at: lib/Basic/Targets/OSTargets.h:279
+                    MacroBuilder &Builder) const override {
+    // Hurd defines; list based off of gcc output
+    DefineStd(Builder, "unix", Opts);
----------------
Missing full stop at the end of the comment.


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:65-66
+
+Hurd::Hurd(const Driver &D, const llvm::Triple &Triple,
+                 const ArgList &Args)
+    : Generic_ELF(D, Triple, Args) {
----------------
Formatting looks off here as well.


================
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
----------------
What GCC logic above?

we currently -> we are currently


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:120
+
+  llvm_unreachable("unsupported architecture");
+}
----------------
This doesn't look unreachable to me?


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:158
+  // target triple.
+
+  if (getTriple().getArch() == llvm::Triple::x86) {
----------------
Spurious newline.


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:161
+    std::string Path = SysRoot + "/usr/include/i386-gnu";
+    if (D.getVFS().exists(Path)) {
+      addExternCSystemInclude(DriverArgs, CC1Args, Path);
----------------
Can elide braces.


================
Comment at: lib/Driver/ToolChains/Hurd.h:22-23
+public:
+  Hurd(const Driver &D, const llvm::Triple &Triple,
+          const llvm::opt::ArgList &Args);
+
----------------
Formatting looks off here; did clang-format produce this?


================
Comment at: lib/Driver/ToolChains/Hurd.h:31-33
+  virtual std::string computeSysRoot() const;
+
+  virtual std::string getDynamicLinker(const llvm::opt::ArgList &Args) const;
----------------
Why are these virtual functions?

`getDynamicLinker()` is implemented but never called in this patch -- is it needed?


================
Comment at: lib/Driver/ToolChains/Hurd.h:35
+
+  std::vector<std::string> ExtraOpts;
+
----------------
This appears to be entirely unused.


Repository:
  rC Clang

https://reviews.llvm.org/D54379