[llvm-commits] [PATCH] Object File Library

[Chris Lattner]

On Nov 11, 2010, at 6:23 PM, Michael Spencer wrote:
> Attached are updated patches to be reviewed. I've split them up into
> the generic API, the COFF and ELF implementations, tool changes, and
> tests.

I just took a look at the first patch.  You're following a very disciplined approach and your code is really clean and beautiful!  Here are some initial thoughts:

First, why the llvm::object namespace?  Conceptually this is part of the MC stuff, so it should use MC prefixes.  If namespaces are the right answer, we should move the MC stuff into a namespace.  By the same argument, instead of lib/Object and include/llvm/Object, it should be under lib/MC/ObjectFile.


+++ b/include/llvm/Object/ObjectFile.h
@@ -0,0 +1,238 @@
+//===- ObjectFile.h - File format independent object file -------*- C++ -*-===//

+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Triple.h"

It is unfortunate to include Triple.h just to get an enum, could it return an unsigned instead?  If you really want it to stay, this doesn't need StringRef.h (we don't follow "include what you use").

+#include "llvm/Support/MemoryBuffer.h"
+#include "llvm/System/Path.h"

Please forward declare MemoryBuffer and sys::Path instead of including them.

+#include <limits>

Please use ~0ULL instead of std::numeric_limits<uint64_t>::max() to eliminate this.


+  typedef uint64_t DataRefImpl;

Out of curiosity, why not void*?  This seems to waste space for 32-bit platforms.


+  class SymbolRef {

Please add doxygen comments above every class, and the major non-obvious methods (like getNMTypeChar).

+  protected:

Unless a class is designed to be subclassed, please use private: (which is the default at the start of a class).

+    ObjectFile(); // = delete
+    ObjectFile(const ObjectFile &other); // = delete

We tend to use a comment saying "// DO NOT IMPLEMENT", but this is also fine and shorter for people who know '0x.

+++ b/lib/Object/ObjectFile.cpp
@@ -0,0 +1,67 @@
+//===- ObjectFile.h - File format independent object file -------*- C++ -*-===//

Please fix the comment. (.h -> .cpp)


+++ b/lib/Object/COFFObjectFile.cpp
@@ -0,0 +1,368 @@
+//===- COFFObjectFile.h - COFF object file implementation -------*- C++ -*-===//

Please fix the comment. (.h -> .cpp)


This is great work, with this changes and getting a decision on the file system/namespace layout, please check in the first patch.

-Chris