Re: [RFA 03/22] Use scoped_restore for ui_file

Simon Marchi
2016-10-01T11:47:26Z

On 2016-10-01 01:22, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simon.marchi@polymtl.ca> writes:
>
> Simon> shared_ptr< scoped_restore<ui_file *> > save_file;
>
> Why shared_ptr and not unique_ptr?

Right, I am not used yet to think about their differences, and which one
applies to a particular situation. unique_ptr is clearler a better
choice.

> Simon> We can't use std::shared_ptr, since it's only in c++11, but I
> think
> Simon> it's just a matter of time before we define our own version of
> it.
>
> ... Pedro's branch has unique_ptr :)

Oh, right!

> Simon> An alternative would be to have a default constructor for
> Simon> scoped_restore, that creates an inactive scoped_restore, and
> then
> Simon> assign it a variable to restore later with acquire(T*
> var)/acquire(T*
> Simon> var, T value) methods or something. I am not sure which one is
> Simon> better.
>
> FWIW Mozilla uses an Option class for this kind of thing.
> It works like:
>
> Option< scoped_restore<ui_file *> > save_file;
> if (mumble) {
> save_file.emplace (make_scoped_restore (&var));
> }
>
> The main advantage of this over *_ptr is that Option contains the
> object, so no heap allocations are required.

That sounds good, it ends up with the same behavior, but implemented
better.

> Simon> To support some use cases where discard_cleanups is used, we
> might
> Simon> need a way to "release" the scoped_restore, which would
> essentially
> Simon> cancel it. So if we have a "release" method, maybe having the
> Simon> symmetrical "acquire" would make sense.
>
> Simon> What do you think?
>
> It seems sensible to me.
>
> One idea to consider is whether it's better to have a separate
> "discardable" variant of scoped_restore (or whatever else); the idea
> being that then the type name serves as a signal to look more closely
> at
> the logic. I think most spots don't need discardable cleanups.

Yep, alsop sounds good.