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