Add ScopedGeneric.

Add ScopedGeneric.

This is intended to be used in a typedef for declaring various types of
scoped resources, like ScopedFD or ScopedGDIObject. It attempts to be like
scoped_ptr but without pointer semantics.

Currently the scoped objects are either custom one-offs that don't support move
semantics and have subtly varying behavior, or they use a pointer like ScopedFD
which is a memory stomp waiting to happen (since you must keep the int* alive
longer than the scoper so it can be dereferenced and closed).

R=ajwong@chromium.org, viettrungluu@chromium.org, ajwong

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256596

[brettw, 2014-03-07 00:56:55]

Minor variations to consider:
 - Moving T to traits rather than having two template args.
 - Moving the null value out of traits and have 3 template args.

[brettw, 2014-03-08 00:54:17]

See https://codereview.chromium.org/191673003/ for an example of using it.

[brettw, 2014-03-08 00:56:54]

I'm now leaning toward making it look more like:
template<typename T, T null_value, typename Deleter>
and replacing the traits with a deleter like scoped_ptr. What do you think?

[ajwong, 2014-03-08 02:28:20]

I think I'm going to need some more baby-step walk through to understand
why the custom deleter isn't sufficient in the ScopedFD case.

Note I'm generally leery of adding more things with move
semantics...they're always so complex...

-Albert



On Fri, Mar 7, 2014 at 4:56 PM, <brettw@chromium.org> wrote:

> I'm now leaning toward making it look more like:
> template<typename T, T null_value, typename Deleter>
> and replacing the traits with a deleter like scoped_ptr. What do you think?
>
> https://codereview.chromium.org/189613002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[viettrungluu, 2014-03-08 03:07:46]

I wrote this elsewhere, but it deserves to be said here:

https://codereview.chromium.org/191673003/diff/1/base/memory/discardable_memo...
base/memory/discardable_memory_allocator_android.cc:70: if (!fd) {
So, it occurs to me that this is extremely confusing.

I think it'd be better for ScopedNullableGeneric to have is_valid/is_invalid
methods. (Though that throws into question the name "Nullable". D'oh.)

[brettw, 2014-03-08 23:05:47]

Maybe removing the ability to say !fd is a good idea, I'll do that.

There was a chromium-dev thread about this a few months ago which I can't find.
The current implementation of ScopedFD in terms of scoped_ptr means there has to
be a pointer in there somewhere. But an FD is just an int.  So ScopedFD takes an
int*, which the closer dereferences. But this means that the creator has to have
some "storage" that outlives the ScopedFD to actually store the file descriptor.
This is very error prone. In the conversion I noticed there's a crash in
https://codereview.chromium.org/191673003/diff/1/content/browser/zygote_host/...
because of this issue.

That's why we need something that acts like a scoped_ptr but isn't a pointer. I
also hope to implement more of our weird custom scoped closers like ScopedHandle
in terms of this.

[brettw, 2014-03-10 17:18:24]

I removed the Testable stuff and added an is_valid() call.

[viettrungluu, 2014-03-10 17:35:55]

LGTM w/nits, but wait for Albert to look at this.

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
File base/scoped_nullable_generic.h (right):

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
base/scoped_nullable_generic.h:20: // It is inteded that you will typedef this
class with an appropriate deleter
s/inteded/intended/

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
base/scoped_nullable_generic.h:113: // object. After this operation, this object
will hold a null pointer, and
s/pointer/value/

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
base/scoped_nullable_generic.h:145: // Forbid comparison of scoped_ptr types. 
If U != T, it totally
scoped_ptr?

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
File base/scoped_nullable_generic_unittest.cc (right):

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
base/scoped_nullable_generic_unittest.cc:119: // Testable.
This comment no longer really makes sense.

https://codereview.chromium.org/189613002/diff/60001/base/scoped_nullable_gen...
base/scoped_nullable_generic_unittest.cc:122: if (!a.is_valid()) {
Probably just EXPECT_TRUE(a.is_valid()); (etc., below).

[brettw, 2014-03-10 17:40:09]

New snap up with Trung's comments addressed.

[awong, 2014-03-11 02:01:43]

Overall, I have to say I'm uncomfortable with this class. It cuts/pastes a large
portion of scoped_ptr and it feels like we're mostly just avoiding an extra
malloc.

Brett, do you think that's enough to be worth adding another moveable type?

https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
File base/scoped_nullable_generic.h (right):

https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
base/scoped_nullable_generic.h:25: // of the scoper (which is easy to mess up).
This doesn't seem quite right to me. Is this class mostly an optimization then? 
I would have expected ScopedFDClose and similar to be implemented like:

struct ScopedFDClose {
  void operator()(int* x) {
    if (x && *x >=0) {
       PCHECK(0 == IGNORE_EINTR(close(*x)));
       delete x;  // -**-- this wasn't here before --*-
    }
  }
};

Usage would then be something like:

scoped_ptr<int, ScopedFDClose> ptr(
   new int(CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path)));

if (*ptr < 0) {
  DoErrorStuff();
}

https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
base/scoped_nullable_generic.h:94: // object, if given. Self-resets are not
allowd as on scoped_ptr. See
nit: allowd -> allowed

[awong, 2014-03-11 02:02:01]

That being said, the implementation looks roughly right.

On 2014/03/11 02:01:43, awong wrote:
> Overall, I have to say I'm uncomfortable with this class. It cuts/pastes a
large
> portion of scoped_ptr and it feels like we're mostly just avoiding an extra
> malloc.
> 
> Brett, do you think that's enough to be worth adding another moveable type?
> 
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> File base/scoped_nullable_generic.h (right):
> 
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> base/scoped_nullable_generic.h:25: // of the scoper (which is easy to mess
up).
> This doesn't seem quite right to me. Is this class mostly an optimization
then? 
> I would have expected ScopedFDClose and similar to be implemented like:
> 
> struct ScopedFDClose {
>   void operator()(int* x) {
>     if (x && *x >=0) {
>        PCHECK(0 == IGNORE_EINTR(close(*x)));
>        delete x;  // -**-- this wasn't here before --*-
>     }
>   }
> };
> 
> Usage would then be something like:
> 
> scoped_ptr<int, ScopedFDClose> ptr(
>    new int(CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path)));
> 
> if (*ptr < 0) {
>   DoErrorStuff();
> }
> 
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> base/scoped_nullable_generic.h:94: // object, if given. Self-resets are not
> allowd as on scoped_ptr. See
> nit: allowd -> allowed

[brettw, 2014-03-11 05:38:06]

https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
File base/scoped_nullable_generic.h (right):

https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
base/scoped_nullable_generic.h:25: // of the scoper (which is easy to mess up).
I really don't like the new(). I'm not that concerned about the performance, but
doing this seems weird and confusing. It seems like awkwardly trying to jam a
scoped file closer into a memory manager class (which it is). If we aren't going
to go this generic approach, I think it would be better to write yet another
one-off scoped for closing file descriptors than do this.

[viettrungluu, 2014-03-11 16:15:41]

On 2014/03/11 05:38:06, brettw wrote:
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> File base/scoped_nullable_generic.h (right):
> 
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> base/scoped_nullable_generic.h:25: // of the scoper (which is easy to mess
up).
> I really don't like the new(). I'm not that concerned about the performance,
but
> doing this seems weird and confusing. It seems like awkwardly trying to jam a
> scoped file closer into a memory manager class (which it is). If we aren't
going
> to go this generic approach, I think it would be better to write yet another
> one-off scoped for closing file descriptors than do this.

Using a scoped_ptr with a custom deleter for things like a "ScopedFD" seems like
a very strange contortion to me.

If you have something like scoped_ptr<int, FDCloseAndDelete>, the obvious thing
to do is then to wonder why the int isn't actually being wrapped in a class so
that the close() is in the destructor. So you do that, and now you have
scoped_ptr<FDWrapper>. I suppose this works, but it's awkward, and you wonder
why FDWrapper (which has only the int FD as a data member and a close() in the
destructor) isn't movable in the first place (since it's now basically a
ScopedFD).

In an ideal world, we'd have scoped_ptr as a specialization of something like
ScopedNullableGeneric instead, but I think we want to keep scoped_ptr lined up
with C++11's unique_ptr.

[awong, 2014-03-11 19:57:01]

On 2014/03/11 16:15:41, viettrungluu wrote:
> On 2014/03/11 05:38:06, brettw wrote:
> >
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> > File base/scoped_nullable_generic.h (right):
> > 
> >
>
https://codereview.chromium.org/189613002/diff/80001/base/scoped_nullable_gen...
> > base/scoped_nullable_generic.h:25: // of the scoper (which is easy to mess
> up).
> > I really don't like the new(). I'm not that concerned about the performance,
> but
> > doing this seems weird and confusing. It seems like awkwardly trying to jam
a
> > scoped file closer into a memory manager class (which it is). If we aren't
> going
> > to go this generic approach, I think it would be better to write yet another
> > one-off scoped for closing file descriptors than do this.
> 
> Using a scoped_ptr with a custom deleter for things like a "ScopedFD" seems
like
> a very strange contortion to me.
> 
> If you have something like scoped_ptr<int, FDCloseAndDelete>, the obvious
thing
> to do is then to wonder why the int isn't actually being wrapped in a class so
> that the close() is in the destructor. So you do that, and now you have
> scoped_ptr<FDWrapper>. I suppose this works, but it's awkward, and you wonder
> why FDWrapper (which has only the int FD as a data member and a close() in the
> destructor) isn't movable in the first place (since it's now basically a
> ScopedFD).

Sorry to keep bouncing this, but I can't quite wrap my head around the
motivations.

I was forcing it a bit with a custom deleter, but I don't buy the slippery
slope. Using scoped_ptr<> for the RAII semantics doesn't seem that uncommon or
unnatural. The trick is to wrap the resource in an object. The ScopedFD just
seems like a badly usage motivate (maybe?) by the want to avoid a new() so let's
ignore that for a bit.

Talking out the current world, imagine the following instead:

class AlbertFile {
 public:
  AlbrertFile(const string& path, int flags);
  ~AlbertFile() {
     /* Shred the bits. Close descriptor. Play nyancat. */
   }
 private:
  int fd_;
  DISALLOW_COPY_AND_ASSIGN(AlbertFile);
};

In C++98, without move semantics, you'd understand exactly what this does. If
you needed move semantics, you'd new() it dump it into a
scoped_ptr<>/unique_ptr<>.

If you replaced this instead with ScopedGeneric, ~AlbertFile() becomes 

  class AlbertFileTraits {
    static int NullValue() {
      return -1;  // Always default to something invalid.
    }

    static void Free(int fd) {
      /* Shred the bits. Close descriptor. Play nyancat. */
    }
  };

The "constructor" is extrinsic to the scoper.  Between these two, it feels like
we've swapped one boilerplate for another. The only concrete gain I see is
avoiding a new().

Is my analysis off?  If so...then I think the real value proposition here is
avoiding a new() in some cases which feels less clearly worth the complexity.

Another options -- which is more radical -- is to be more okay with people
creating move-only types.

-Albert


> In an ideal world, we'd have scoped_ptr as a specialization of something like
> ScopedNullableGeneric instead, but I think we want to keep scoped_ptr lined up
> with C++11's unique_ptr.

[brettw, 2014-03-11 22:30:39]

If your AlbertFile example was typical, I think your example of small wrapper
classes would be more valid. But a typical example looks more like:
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_gd...
which is 40 lines implementing various set and clearing. This helper class "kind
of" has scoped pointer semantics, but not all of them, and everybody that
implements these does a slightly different version of the same thing.

You seem to be mostly worried about the move stuff. I don't actually care that
much about the move semantics in particular. I just want various scoper
doohickies to behave consistently.

[awong, 2014-03-11 22:59:06]

LGTM/wits

On 2014/03/11 22:30:39, brettw wrote:
> If your AlbertFile example was typical, I think your example of small wrapper
> classes would be more valid. But a typical example looks more like:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/scoped_gd...
> which is 40 lines implementing various set and clearing. This helper class
"kind
> of" has scoped pointer semantics, but not all of them, and everybody that
> implements these does a slightly different version of the same thing.
> 
> You seem to be mostly worried about the move stuff. I don't actually care that
> much about the move semantics in particular. I just want various scoper
> doohickies to behave consistently.

Okay...convinced. Can you update the review description to focus more on
unifying the scopers and their need to rewrite set, reset, comparisons, etc.
Right now it focuses on the delta from scoped_ptr<> which is very useful, but
doesn't explain the "why."

BTW, one bit I'm slightly worried about is the need to write a bunch of Traits
objects. It makes me worry about ODR violations a bit because people forget that
placing something like the class declaration for a set of Traits in a .cc file
doesn't exempt it from ODR problems unless it's also wrapped in an anonymous
namespace. The same problem exists for all Traits-style templates and for the
Deleter in scoped_ptr<>...however it's much rarer to use those. Not sure if
there's any way to fix it, but wanted to voice the concern.

-Albert

[brettw, 2014-03-11 23:37:29]

As per offline discussion with awong, renamed to ScopedGeneric and renamed
NullValid to InvalidValue

[awong, 2014-03-11 23:40:48]

still LGTM + 1 nit

https://codereview.chromium.org/189613002/diff/120001/base/scoped_generic.h
File base/scoped_generic.h (right):

https://codereview.chromium.org/189613002/diff/120001/base/scoped_generic.h#n...
base/scoped_generic.h:18: // object has some kind of "null" value.
null -> invalid

[brettw, 2014-03-12 19:22:20]

Committed patchset #9 manually as r256596 (presubmit successful).