[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