[PATCH] [lld] Fix the ELF shared library build targets

Rui Ueyama ruiu at google.com
Thu Jan 22 13:49:24 PST 2015


In this patch you moved lib/ReaderWriter/ELF/<arch>/<arch>LinkingContext to lib/ReaderWriter/ELF. Why did you have to do that? Because of that change, the size of this diff is large, so it is not easy to find the answer myself. Could you elaborate that a bit?


REPOSITORY
  rL LLVM

================
Comment at: include/lld/ReaderWriter/ELF/Targets.h:21
@@ +20,2 @@
+
+#endif
----------------
I don't agree that this is a good idea. We should generally avoid transitive inclusions. If a source file depends on a bunch of headers, it actually depends on all of them. Hiding that dependency by replacing with one #include doesn't make things clean but makes things slightly worse.

================
Comment at: lib/Driver/GnuLdDriver.cpp:343
@@ +342,3 @@
+  default:
+    return nullptr;
+  }
----------------
Legitimate code should never reach here, no? If so, please use llvm_unreachable.

http://reviews.llvm.org/D7119

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list