[LLVMdev] object construction patterns and unique_ptr

[Jeffrey Yasskin]

On Wed, Jun 20, 2012 at 12:36 PM, Nick Kledzik <kledzik at apple.com> wrote:
> On Jun 20, 2012, at 12:28 AM, Chandler Carruth wrote:
>> On Tue, Jun 19, 2012 at 11:18 AM, Jeffrey Yasskin <jyasskin at google.com> wrote:
>> On Jun 18, 2012 1:24 PM, "Nick Kledzik" <kledzik at apple.com> wrote:
>> >
>> > 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);
>> >
>>
>> I suggest an error_or<T> class that can be combined with any result type, not just File. It could be implemented with a fully generic variant class, but the interface would likely be better with some specialization to errors.
>
> I too think that error_or<T> is better than std::pair<error_code, T>:
> * more compact
> * can name fields (e.g. error, object instead of first, second)
> * can embed asserts that object field is not accessed if there is an error.
>
> But there are some drawbacks:
> * lld uses C++11, so we thought we'd add explicit ownership by using std::unique_ptr.  That means we'd need to layer the templates (e.g. error_or<std::unique_ptr<lld::File>>), or forgo using unique_ptr, or cause error_or to always internally use unique_ptr.

Consider *all* the C++11 features, not just unique_ptr. As Howard
suggested at http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-June/050867.html,
in C++11, I would probably make lld::File a move-only type rather than
asking users to manage its lifetime by wrapping it in unique_ptr.
error_or<lld::File> should work fine in that model.

If lld::File is an abstract base class, then you need the unique_ptr
... but if File is used enough you might win by writing a
type-erasing, move-only wrapper. Talk to Howard if you need help doing
that.

Even if the type-erasing wrapper isn't worth doing, layered templates
aren't the end of the world. They have the benefit that users can see
exactly what they're getting. If you prefer to hide the details of the
type, a typedef may be enough.

> * We still have not figured out how to encapsulate errors for object file parsing.  The last thread was an enum *and* and a string.

Yes, you may need to iterate on exactly what fields error_or<T>
contains when it's representing an error. I wouldn't assume it can
just be a std::error_code.

> Instead of thinking of FileError as a degenerate File with an error_code slapped on.  Think of FileError as an object encapsulating the attempt to read and parse the file.

Names are important and worth spending time on. If you want people to
think of a class as a degenerate File with an error_code slapped on,
name it "FileError" (actually, "FileError" just sounds like the
error_code part, without the File). If you want people to think of it
as the result of an attempt to read and parse a file, name it
something related to reading and parsing a file.

HTH,
Jeffrey