[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

[Michael Spencer]

On Mon, Nov 15, 2010 at 5:14 PM, Chris Lattner <clattner at apple.com> wrote:
> 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?

I'll get rid of Path. I decided not to use unsigned because it would
require adding casts in other places, although this can change.

>> --- 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).

OK.

>> +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

I think for historical reasons. I'll change it.

- Michael Spencer