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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
