[llvm-commits] [llvm] r119107 - in /llvm/trunk: CMakeLists.txt include/llvm/Object/ include/llvm/Object/ObjectFile.h lib/Makefile lib/Object/ lib/Object/CMakeLists.txt lib/Object/ObjectFile.cpp

[Chris Lattner]

On Nov 14, 2010, at 7:21 PM, Michael J. Spencer wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=119107&view=rev
> Log:
> Add LLVMObject Library.

Looks nice, some minor stuff:

> +#include "llvm/ADT/Triple.h"
> +#include "llvm/System/Path.h"

You should be able to forward declare sys::Path to avoid the Path #include.  It take it that you decided not to use an 'unsigned' for the getArch() return value?

> --- llvm/trunk/lib/Object/ObjectFile.cpp (added)
> +++ llvm/trunk/lib/Object/ObjectFile.cpp Sun Nov 14 21:21:41 2010
...

> +#include "llvm/Object/ObjectFile.h"
> +#include "llvm/Support/ErrorHandling.h"
> +#include "llvm/Support/MemoryBuffer.h"
> +
> +namespace llvm {
> +namespace object {

Instead of nesting the entire contents of the file in the namespace, please use "using namespace" to pull them in scope.  using namespace is a good solution for .cpp files (but obviously not for headers).

> +ObjectFile *ObjectFile::createObjectFile(const sys::Path &ObjectPath) {
> +  return createObjectFile(MemoryBuffer::getFile(ObjectPath.c_str()));
> +}

Do we really want to encourage/force sys::path here?  Why not take a stringref?

-Chris