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

[Michael Spencer]

================
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"
+
----------------
Shankar Kalpathi Easwaran wrote:
> Why isnt LinkerOptions included ?
Because it's not needed. See: http://llvm.org/docs/CodingStandards.html#minimal-list-of-includes

================
Comment at: include/lld/Core/TargetInfo.h:49-50
@@ +48,4 @@
+  llvm::Triple getTriple() const;
+  virtual bool is64Bits() const;
+  virtual bool isLittleEndian() const;
+
----------------
Shankar Kalpathi Easwaran wrote:
> should these be pure virtual ?
It's easy enough to provide a default implementation of them using the Triple.

================
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;
+  }
+
----------------
Shankar Kalpathi Easwaran wrote:
> There is no error returned from here, why are you using ErrorOr here ?
Because other implementations can return an error.

================
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();
 
----------------
Shankar Kalpathi Easwaran wrote:
> Why is Target being used, shouldnt everything be using TargetInfo ?
This is the driver. Target creates and owns the TargetInfo given the linker options.

================
Comment at: include/lld/ReaderWriter/ELFTargetInfo.h:23
@@ +22,3 @@
+  uint16_t getOutputType() const;
+  uint16_t getOutputMachine() const;
+
----------------
Shankar Kalpathi Easwaran wrote:
> This should return the type of e_machine ? Do we have typedefs in ELF.h for all of the header flags ?
The type of e_machine is Elf_Half. The ELF standard doesn't provide enum names.

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


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