[lld] r226225 - Simplify.

[Chandler Carruth]

On Sat, Jan 17, 2015 at 6:15 AM, Jean-Daniel Dupas <dev at xenonium.com> wrote:

> > Le 16 janv. 2015 à 18:48, David Blaikie <dblaikie at gmail.com> a écrit :
> >
> >
> >
> > On Fri, Jan 16, 2015 at 3:02 AM, Jean-Daniel Dupas <dev at xenonium.com>
> wrote:
> > Just a question not directly related to that change, but similar. Is
> there something that prevent the use of vector.emplace_back() in the lld
> code base (compiler or supported c++ library issue) ?
> >
> > There is some places where we just create a new object and push it back
> in a vector and could simplify them using emplace_back() instead.
> >
> > Personally I wouldn't use "emplace_back(new T())" on a container of
> unique_ptr<T> because of exception safety, etc (though we don't use
> exceptions, it's habits that make me more comfortable reading code, etc).
> The risk is that emplace_back could fail (when trying to allocate more
> space, for example) and then leak the T* before it ever created a
> unique_ptr<T> around it.
> >
> > So I prefer "push_back(make_unique<T>())" (& in general I prefer
> make_unique wherever possible, it means I don't have to think hard about
> whether the 'new' is used safely, etc)
> >
>
> Of course. I was thinking about simplifying construct like:
>
>   Section temp;
>   file.sections.push_back(std::move(temp));
>
> or
>
>   pages.push_back(UnwindInfoPage());
>

Personally I find emplace_back significantly harder to read for little
benefit. It hides the constructor call behind a fairly magical forwarding
function. I would generally rather see a constructor call explicitly. The
only time this bothers me is when it isn't really a constructor at all but
just an aggregate thing. There, I would rather use list initialization, but
we can't because of MSVC 2012.

The times when emplace_back seems useful is when there is a specific reason
that you need to construct things in place. But that comes up quite rarely
in my experience.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150117/f8f7dfb2/attachment.html>