[PATCH] D54379: Add Hurd toolchain support to Clang

[Kristina Brooks via Phabricator via cfe-commits]

kristina added a comment.

Added first batch of comments regarding the patch. Some style, some semantics-related.



================
Comment at: lib/Basic/Targets/OSTargets.h:283
+    Builder.defineMacro("__GLIBC__");
+    Builder.defineMacro("__ELF__");
+    if (Opts.POSIXThreads)
----------------
`__MACH__` and `__HURD__` seem appropriate? Apple Mach (Darwin/XNU) uses `__MACH__` with `__APPLE__`, Hurd should probably follow a similar convention, I don't think there are many places aside from XNU build where `__MACH__` is used on its own.


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:72
+                 const ArgList &Args)
+    : Generic_ELF(D, Triple, Args) { }
+
----------------
No space here.


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:78
+
+  return std::string();
+}
----------------
I'm not quite sure I like this. Also early return should be for the "bad" case, not for the good case, at least IMO, this is not a huge issue but I'll see what others say. I think this may just be subjective.


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:84
+  const Driver &D = getDriver();
+  std::string SysRoot = computeSysRoot();
+
----------------
Move semantics? Or is this guaranteed elision (I'm not sure)?


================
Comment at: lib/Driver/ToolChains/Hurd.cpp:118
+  const StringRef X86MultiarchIncludeDirs[] = {
+      "/usr/include/i386-gnu"};
+
----------------
Until needed I wouldn't use an array here, also drop const.


Repository:
  rC Clang

https://reviews.llvm.org/D54379