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

Filipe Cabecinhas filcab+llvm.phabricator at gmail.com
Fri Feb 20 23:01:18 PST 2015


I have some nits.
Sorry about getting some of the surrounding lines that I ended up not commenting on the other patch, but I figured, since it's from the same revision and you're changing that code… Might as well comment on it.

Thanks for fixing this!


REPOSITORY
  rL LLVM

================
Comment at: include/lld/ReaderWriter/ELFTargets.h:18
@@ -17,6 +17,3 @@
 
-#define LLVM_TARGET(TargetName) \
-  class TargetName##LinkingContext final : public ELFLinkingContext { \
-  public: \
-    static std::unique_ptr<ELFLinkingContext> create(llvm::Triple); \
-  };
+#define LLVM_TARGET(name) \
+  std::unique_ptr<ELFLinkingContext> create##name##LinkingContext(llvm::Triple);
----------------
Why LLVM_TARGET? What does this have to do (directly) with llvm and its (enabled?) targets?
Why not ELF_TARGET (or ELF_ARCH), or something like that, since it's about targets supported for linking ELF files?

================
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) \
----------------
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”?

================
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)
----------------
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?

================
Comment at: lib/Driver/GnuLdDriver.cpp:344
@@ -343,2 +343,2 @@
   LLVM_TARGET(ARM)
   LLVM_TARGET(Hexagon)
----------------
We have this list of LLVM_TARGET expansions at least twice. Do we want something like a .def file?

http://reviews.llvm.org/D7807

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






More information about the llvm-commits mailing list