[LLVMdev] object construction patterns and unique_ptr

[Howard Hinnant]

On Jun 15, 2012, at 6:48 PM, Nick Kledzik wrote:

> In lld we have a Reader class which is a factory that takes .o file(s) and produces an in memory lld::File object(s).  But in doing so, there could be I/O errors (file not readable) or file may be malformed.  We are also using C++11 in lld, so we use std::unique_ptr for managing object ownership.  
> 
> The Reader class currently has an interface that can be simplified down to:
> 
>    virtual error_code readFile(StringRef path, std::unique_ptr<lld::File> &result);
> 
> But this interface has become awkward to use. There are two "return" values.  This method is a unique_ptr "source" but the use of a by-refernce parameter means you have to start with an uninitialized unique_ptr and the readFile() has to std::move() into it.   unique_ptr is much nicer to use as a return value.
> 
> An other issue is that since llvm::error_code was designed to return a fixed set of strings, there is no way to return a dynamic error message (e.g. "relocation 27 is invalid"). 
> 
> In asking around for clean models on how to do this better, I've heard two interesting ideas:
> 
> 1) Incorporate the error_code and string message into the File class.  That is the factory method always returns a File object. But there is a method on File objects to query if they are valid or not.  If they are not, there is a method to get the error_code and/or error string.
> 
> 2) A variation on this is to have a new subclass of File called FileError.  If the Reader cannot construct a real FileFoo object because of some error, it instead constructs a FileError object and returns that.  The FileError object has methods to get the error_code and/or error string.
> 
> With both these ideas the Reader method is:
> 
>    virtual std::unique_ptr<lld::File> readFile(StringRef path);
> 
> which is much easier to use.
> 
> 
> Any recommendations on a good way to design this?

I don't know anything about lld::File.  But I was just glancing at this and I started to wonder:  What is the added benefit of unique_ptr here?  Why not:

   virtual lld::File readFile(StringRef path);

?

I.e. lld::File can be move-only and manage its own resources.  The only reason to use unique_ptr<lld::File> is if lld::File is a base class of what the unique_ptr actually points to.  Or if you want to have a nullptr state that isn't easily integrated into lld::File itself.  But it sounds like neither of these would be the case if lld::File incorporated its own error state.

Howard