llvm dev - Jun 2012 - [LLVMdev] object construction patterns and unique_ptr

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?

-Nick

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

make more sense?

-Chris

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

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