[LLVMdev] object construction patterns and unique_ptr

[Nick Kledzik]

On Jun 16, 2012, at 3:51 PM, Chris Lattner wrote:
> On Jun 15, 2012, at 3: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.
> 
> Not a c++'11 expert here, but does a returning an std::tuple work?
I talked to Howard about that.  It does work, but it is cumbersome.  Either:

std::tuple<std::unique_ptr<lld::File>, error_code>  result = reader.readFile(path);

Or you use std::tie() but then you still have to declare the two local variables and tie them together.  You could define a wrapper class for the tuple, but then you realize it would be simpler to push the error_code into the File and get rid of the wrapper class.

>> 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"). 
> 
> I'm pretty sure that the linker can specify a custom error_category with whatever information you want, and then return an error_code put with that.  Michael would know for sure, but it is definitely in the design of error_code for it to be extensible.

>From what I can tell error_code is about wrapping out OS specific "int" error codes, and how to map those to static strings.  I suppose you could abuse it by having a error_category which has a static vector or recently dynamically generated strings and the int "error code" is an index into that vector.  

Clang has this great Diagnostics infrastructure for showing where the error occurred in the source file.  Does developing something like that make sense for object files whose source is not human readable?

>> 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?
> 
> Would an API like:
> 
>   virtual std::unique_ptr<lld::File> readFile(StringRef path, error_code &ErrorInfo);

That is a common pattern, used for instance in the bit code reader.  If we can unify the OS level error and the syntax/semantic errors from parsing object files, this pattern might work.  

One other thought that leans me towards integrating the error code/msg into the File object is that it works cleaner when you start adding multithreading to the linker.  When you have multiple threads going, each reading an object file and one of them returns an error, you need to cleaning shut down the other threads, and return the error.  With the error status integrated into the File object, all you do is have each thread append it resulting File object onto a list, then after all threads are joined, run through the list once to see if any say they encountered an error.  If the error_code is returned separately from the File, you have to create some data structure to bind together the error_code with which file path could not be parsed.

-Nick