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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


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


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


================
Comment at: lld/ELF/Target.h:21
 namespace lld {
+std::string toString(uint32_t RelType);
+
----------------
jhenderson wrote:
> 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?
I agree that this is too generic, but since it's not new code, let's keep it as-is.


================
Comment at: lld/ELF/Target.h:118
+
+std::string getErrorLocation(const uint8_t *Loc);
+
----------------
jhenderson wrote:
> Should this be in a different file? Not sure where though on a quick glance.
Maybe. But let's not change too much in code-moving change.


================
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");
----------------
jhenderson wrote:
> 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.
Ditto


https://reviews.llvm.org/D34222