[llvm-commits] [PATCH] [lld] Add TargetInfo abstraction.

[Shankar Kalpathi Easwaran]

================
Comment at: include/lld/Core/TargetInfo.h:67-73
@@ +66,9 @@
+
+  virtual ErrorOr<std::string> stringFromRelocKind(uint32_t kind) const {
+    std::string s;
+    llvm::raw_string_ostream str(s);
+    str << kind;
+    str.flush();
+    return s;
+  }
+
----------------
There is no error returned from here, why are you using ErrorOr here ?

================
Comment at: include/lld/Core/TargetInfo.h:20-25
@@ +19,8 @@
+
+#include "lld/Core/Error.h"
+#include "lld/Core/ErrorOr.h"
+#include "lld/Core/LLVM.h"
+
+#include "llvm/Support/DataTypes.h"
+#include "llvm/Support/raw_ostream.h"
+
----------------
Why isnt LinkerOptions included ?

================
Comment at: include/lld/Driver/Target.h:24-29
@@ -23,8 +23,8 @@
 
 namespace lld {
 /// \brief Represents a specific target.
 class Target {
 public:
   Target(const LinkerOptions &lo) : _options(lo) {}
   virtual ~Target();
 
----------------
Why is Target being used, shouldnt everything be using TargetInfo ?

================
Comment at: include/lld/ReaderWriter/ELFTargetInfo.h:23
@@ +22,3 @@
+  uint16_t getOutputType() const;
+  uint16_t getOutputMachine() const;
+
----------------
This should return the type of e_machine ? Do we have typedefs in ELF.h for all of the header flags ?

================
Comment at: include/lld/ReaderWriter/ELFTargetInfo.h:22-23
@@ +21,4 @@
+
+  uint16_t getOutputType() const;
+  uint16_t getOutputMachine() const;
+
----------------
Should this be changed to getCPUType() and getCPUSubtype as how is it in MachO, because on ELF also we have sparc, sparcv9 

================
Comment at: include/lld/Core/TargetInfo.h:49-50
@@ +48,4 @@
+  llvm::Triple getTriple() const;
+  virtual bool is64Bits() const;
+  virtual bool isLittleEndian() const;
+
----------------
should these be pure virtual ?


http://llvm-reviews.chandlerc.com/D311