[PATCH] D34222: Split Target.cpp into small files.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I like the idea here. My main concern is that I see several generic-looking functions in Target.h. Personally, I would kind of expect these functions to be in other locations, and Target.h only to include the interface that each of the different targets implements.



================
Comment at: lld/ELF/Target.h:15
 #include "InputSection.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Object/ELF.h"
----------------
Unused header?


================
Comment at: lld/ELF/Target.h:18
 
 #include <memory>
 
----------------
Unused header?


================
Comment at: lld/ELF/Target.h:21
 namespace lld {
+std::string toString(uint32_t RelType);
+
----------------
This function is way too generically named to be in the LLD namespace. Perhaps something like relocationTypeToString. Also, it doesn't feel like it belongs in Target.h. Maybe Relocation.h?


================
Comment at: lld/ELF/Target.h:118
+
+std::string getErrorLocation(const uint8_t *Loc);
+
----------------
Should this be in a different file? Not sure where though on a quick glance.


================
Comment at: lld/ELF/Target.h:127-130
+static void checkInt(uint8_t *Loc, int64_t V, uint32_t Type) {
+  if (!llvm::isInt<N>(V))
+    error(getErrorLocation(Loc) + "relocation " + lld::toString(Type) +
+          " out of range");
----------------
It feels to me like this and the other check* functions don't really belong in Target.h, since they're nothing to do with the target. It seems like they should really be in Relocations.h.

They're also marked as static, but in a header file, which looks wrong to me.


https://reviews.llvm.org/D34222