[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 15, 2010, at 2:51 PM, Michael Spencer wrote:

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

Sounds good, thanks!

-Chris

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