[PATCH] [lld] Remove duplicate class definitions of ELF LinkingContexts

[Filipe Cabecinhas]

I'm all for integrating lld into llvm (I can accept either version), but it doesn't make sense to start having some scattered pieces of code to handle it, if there has been no decision on integrating lld into llvm.


REPOSITORY
  rL LLVM

================
Comment at: lib/Driver/GnuLdDriver.cpp:339
@@ -338,2 +338,3 @@
   std::unique_ptr<ELFLinkingContext> p;
   // FIXME: #include "llvm/Config/Targets.def"
+#define LLVM_TARGET(name) \
----------------
garious wrote:
> filcab wrote:
> > IIUC, nothing in lld depends on llvm's enabled targets (except some MIPS tests, IIRC). Why wouldn't this just depend on “targets lld can handle”?
> Because it could be both "targets lld can handle" and "targets enabled by the llvm build".  You can mix the two by adding a 'create' function for each llvm-supported target and returning nullptr to indicate "unsupported by lld".
But we're not doing that now. lld fully implements everything, without caring about which llvm targets are enabled, which means we don't have the case where we want to selectively compile lld's support depending on what support is compiled in llvm).

I would prefer to have a clean implementation now and, if we ever change and start depending on the llvm targets directly, change to another clean implementation. Right now, having those tests in the linking contexts seem useless (I might be missing something, of course).


================
Comment at: lib/Driver/GnuLdDriver.cpp:341
@@ -342,1 +340,3 @@
+#define LLVM_TARGET(name) \
+  if ((p = elf::create##name##LinkingContext(triple))) return p;
   LLVM_TARGET(AArch64)
----------------
garious wrote:
> filcab wrote:
> > Why not switch on the Triple, and call the appropriate function (which wouldn't have the Triple check, but (possibly) an assert), instead of doing the whole “try to create*()” dance?
> So that we can enable only select targets via LLVM_TARGETS_TO_BUILD.
Even if that's a future feature, it still makes the name misleading, right now.

http://reviews.llvm.org/D7807

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