718938 - Add RAII helper template to MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Mike Hommey [:glandium]]

Attachment: rudimentary RAII helper

In bug 683127, I implemented a rudimentary RAII helper template, that could probably be grown and included in MFBT to be used in other places in Gecko. The attached file contains the implementation from bug 683127.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Feature-wise, the code I see looks sufficient for my current needs. Does anyone see any glaring missing feature, or could we start that this patch is code-complete, and add documentation, headers, and prepare it for landing?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Notes:
- I can see some users (e.g. JSAPI) who may need some more features (e.g. to use JS_free), but I believe that this can go in another patch;
- shouldn't we use MOZILLA_GUARD_* macros, as per https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla ?

Comment 3

[Mike Hommey [:glandium]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Notes:
> - I can see some users (e.g. JSAPI) who may need some more features (e.g. to
> use JS_free), but I believe that this can go in another patch;

They would just need a Trait for that, and that should probably stay in JS, not be part of MFBT.

> - shouldn't we use MOZILLA_GUARD_* macros, as per
> https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla ?

Sounds like a good idea.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Mike Hommey [:glandium] from comment #3)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> > Notes:
> > - I can see some users (e.g. JSAPI) who may need some more features (e.g. to
> > use JS_free), but I believe that this can go in another patch;
> 
> They would just need a Trait for that, and that should probably stay in JS,
> not be part of MFBT.

I just meant that we should avoid, if possible, a class that would be incompatible with such uses. Note that JS_free takes two arguments (context and pointer), not one – writing a trait to embed this is certainly possible, and might actually be fun to do.

> 
> > - shouldn't we use MOZILLA_GUARD_* macros, as per
> > https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla ?
> 
> Sounds like a good idea.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> - shouldn't we use MOZILLA_GUARD_* macros, as per
> https://developer.mozilla.org/en/Using_RAII_classes_in_Mozilla ?

MOZ_GUARD_* now, btw

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Here is a patchified version. I have also added MOZ_GUARD_* and a few lines of documentation.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

(same one, with the file)

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 594534 [details] [diff] [review]
Rudimentary RAII helper

Review of attachment 594534 [details] [diff] [review]:
-----------------------------------------------------------------

I look at this, and I wonder just how much of it actually provides a useful abstraction.  It feels like a lot of RAII boilerplate to me that would be more clear written out longhand.

It's fairly easy to understand a class with a constructor and destructor.  Traits-based classes are rather more complicated, because you have several different levels of indirection to pass through to understand things.  They have their place, certainly; I'm just not sure it's here.  I'll pass this review along and see what other people say.  I'm inclined to say people should just write out their RAII classes from scratch, myself.

I should note that if this really is desirable, the various class specializations should be in separate headers.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Additional issue: at the moment, this does not compile with VC++. Mike and me are on it.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Attaching a fix for VC++. For some reason, VC++ seemed to get confused by type variable names. Just renaming 'T' to 'Type' seems to fix the issue, though.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Adding a tiny fix. Apologies for the noise.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 597607 [details] [diff] [review]
Rudimentary RAII helper

This looks good to me.  In addition to the comments below, I'd like to
test drive this by rewriting some gecko helpers in terms of this.  At
least,

 - AutoFdClose / ScopedClose
 - ScopedXFree

Would also like to see this used on the JS side.  Up to Jeff/Luke
though.

>diff --git a/mfbt/AutoClean.h b/mfbt/AutoClean.h

Nit: this should be "ScopedClean".  "Auto" in this context usually
implies it's only allocated in automatic storage.  But a common usage
for this kind of helper is as a class member.

Also, s/Auto/Scoped/ for the specializations below.

>+template <typename Traits>
>+class AutoClean

(ScopedClean)

>+  operator const T&() const { return value; }

You'll also need a non-const coercion operator for pointers.

>+  const T& operator->() const { return value; }

This doesn't make sense for the fd example, but I guess it's the only
one here.

>+  const T& get() const { return value; }
>+

non-const too

>+  T forget()
>+  {

Nit: style in mfbt is 

  T forget() {

>+    T _value = value;

Nit: s/_value/tmp/.

>+  bool operator ==(const T& other) const

Nit: style is "operator==", no space.

>+#if defined(XP_UNIX)
>+
>+ * AutoCloseFD is a RAII wrapper for POSIX file descriptors
>+ */
>+struct AutoCloseFDTraits

If js/src isn't going to use this let's just remove it from here.  The
ifdef'ery and include of unistd.h here are a little iffy.

Comment 15

[David Teller [:Yoric] - still alive but not very active]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #14)
> Comment on attachment 597607 [details] [diff] [review]
> Rudimentary RAII helper
> 
> This looks good to me.  In addition to the comments below, I'd like to
> test drive this by rewriting some gecko helpers in terms of this.  At
> least,
> 
>  - AutoFdClose / ScopedClose
>  - ScopedXFree
> 
> Would also like to see this used on the JS side.  Up to Jeff/Luke
> though.

Sounds like followup bugs.
See also bug 720771 for a teaser of using this in js/src.

> >diff --git a/mfbt/AutoClean.h b/mfbt/AutoClean.h
> 
> Nit: this should be "ScopedClean".  "Auto" in this context usually
> implies it's only allocated in automatic storage.  But a common usage
> for this kind of helper is as a class member.
> 
> Also, s/Auto/Scoped/ for the specializations below.

I agree that it is clearer.


> Nit: style in mfbt is 
> 
>   T forget() {

Gasp. How many different styles do we have on mozilla-central?

> >+#if defined(XP_UNIX)
> >+
> >+ * AutoCloseFD is a RAII wrapper for POSIX file descriptors
> >+ */
> >+struct AutoCloseFDTraits
> 
> If js/src isn't going to use this let's just remove it from here.  The
> ifdef'ery and include of unistd.h here are a little iffy.

So, where would you put this?

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Comment 17

[David Teller [:Yoric] - still alive but not very active]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #14)
> Comment on attachment 597607 [details] [diff] [review]
> Rudimentary RAII helper
> 
> This looks good to me.  In addition to the comments below, I'd like to
> test drive this by rewriting some gecko helpers in terms of this.  At
> least,
> 
>  - AutoFdClose / ScopedClose
>  - ScopedXFree
> 
> Would also like to see this used on the JS side.  Up to Jeff/Luke
> though.

Actually, I may have misunderstood your comment. Do you mean that you want us to rewrite uses of AutoFDClose / ScopedClose, ScopedXFree based on Scoped.h before marking this r+ ? I am willing to do some of it once we are satisfied with the API.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 598186 [details] [diff] [review]
Rudimentary RAII helper

Review of attachment 598186 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the review delay.  Great utility, I definitely think we can use it.  r+ with changes requested below.

To wit, I just tried and C++11 lambdas would allow us to write:
  
  T resource = GetResource();
  auto release = OnExitScope([]() { ReleaseResource(resource); });

That's hot.  How long until we can depend on lambda compiler support?

::: mfbt/Scoped.h
@@ +15,5 @@
> + *     typedef value_type type;
> + *     // Returns the value corresponding to the uninitialized or freed state
> + *     const static type None();
> + *     // Cleans up resources corresponding to the wrapped value
> + *     const static void clean(type);

I really liked the change from AutoClean to Scoped.  Could we get a similar change from 'clean' to something more canonical, like 'release' or 'finish' ?

@@ +68,5 @@
> +   * not want to trigger clean-up, you should first invoke |forget|.
> +   *
> +   * @return this
> +   */
> +  Scoped<Traits>& operator=(const T& other) {

With effectful helpers like Scoped, I think it's best to be very explicit about when the effect is applied.  For example, as is, 'Scoped<T> s = t;' compiles and technically means 'Scoped<T> s = Scoped<T>(t);' (construct temporary, copy temporary, destroy temporary) and it is only the widely-implemented semantics-altering copy-ellision optimization allowed by C++ (en.wikipedia.org/wiki/Copy_elision) that gives you the desired 'Scoped<T> s(t);'.  That should scare you :)  To this end, could you:
 - slap 'explicit' on all the current constructors
 - define the copy constructor with 'explicit' (as is, it is implicitly defined)
 - MOZ_DELETE operator= and define some equivalent named, e.g., 'reset'

@@ +92,5 @@
> +#define SCOPEDCLEAN_TEMPLATE(name, Traits)                      \
> +template <typename Type>                                      \
> +struct name: public Scoped<Traits<Type> >                  \
> +{                                                             \
> +  typedef Scoped<Traits<Type> > Super;                     \

\ alignment

Comment 19

[Luke Wagner [:luke]]

Oops, I meant "OnExitScope([=](){ ReleaseResource(resource); })" :)

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #14)
> > Comment on attachment 597607 [details] [diff] [review]
> > Rudimentary RAII helper
> > Nit: style in mfbt is 
> > 
> >   T forget() {
> 
> Gasp. How many different styles do we have on mozilla-central?
> 

You must be new here ;).

> > >+#if defined(XP_UNIX)
> > >+
> > >+ * AutoCloseFD is a RAII wrapper for POSIX file descriptors
> > >+ */
> > >+struct AutoCloseFDTraits
> > 
> > If js/src isn't going to use this let's just remove it from here.  The
> > ifdef'ery and include of unistd.h here are a little iffy.
> 
> So, where would you put this?

xpcom/glue/FileUtils.h until js/src needs it.

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #14)
> > Comment on attachment 597607 [details] [diff] [review]
> > Rudimentary RAII helper
> > 
> > This looks good to me.  In addition to the comments below, I'd like to
> > test drive this by rewriting some gecko helpers in terms of this.  At
> > least,
> > 
> >  - AutoFdClose / ScopedClose
> >  - ScopedXFree
> > 
> > Would also like to see this used on the JS side.  Up to Jeff/Luke
> > though.
> 
> Actually, I may have misunderstood your comment. Do you mean that you want
> us to rewrite uses of AutoFDClose / ScopedClose, ScopedXFree based on
> Scoped.h before marking this r+ ? I am willing to do some of it once we are
> satisfied with the API.

Yes.

Comment 21

[Mike Hommey [:glandium]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #20)
> > So, where would you put this?
> 
> xpcom/glue/FileUtils.h until js/src needs it.

Note there is already an AutoFDClose in FileUtils.h, for the NSPR's PRFileDesc, and it makes sense that xpcom's version does so. However, if js/src ends up needing an autoclose, I doubt it would need one that depends on NSPR. As a general rule, I think mfbt shouldn't introduce dependencies on NSPR.

Also note that these templates are coming from mozglue/linker/Utils.h, which means the linker uses them (except AutoFreePtr).

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 598186 [details] [diff] [review]
Rudimentary RAII helper

r=me with deferring ScopedCloseFD per above

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Those should be rewritten to use this helper.

Comment 24

[David Teller [:Yoric] - still alive but not very active]

(In reply to Luke Wagner [:luke] from comment #18)
> With effectful helpers like Scoped, I think it's best to be very explicit
> about when the effect is applied.  For example, as is, 'Scoped<T> s = t;'
> compiles and technically means 'Scoped<T> s = Scoped<T>(t);' (construct
> temporary, copy temporary, destroy temporary) and it is only the
> widely-implemented semantics-altering copy-ellision optimization allowed by
> C++ (en.wikipedia.org/wiki/Copy_elision) that gives you the desired
> 'Scoped<T> s(t);'.  That should scare you :)  To this end, could you:
>  - slap 'explicit' on all the current constructors
>  - define the copy constructor with 'explicit' (as is, it is implicitly
> defined)

Actually, do we want a copy constructor at all? Or should I make it private?

>  - MOZ_DELETE operator= and define some equivalent named, e.g., 'reset'

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Attached a new version that takes into account the various suggestions above.

(In reply to Luke Wagner [:luke] from comment #18)
> Comment on attachment 598186 [details] [diff] [review]
> Rudimentary RAII helper
> 
> Review of attachment 598186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the review delay.  Great utility, I definitely think we can use
> it.  r+ with changes requested below.
> 
> To wit, I just tried and C++11 lambdas would allow us to write:
>   
>   T resource = GetResource();
>   auto release = OnExitScope([]() { ReleaseResource(resource); });
> 
> That's hot.  How long until we can depend on lambda compiler support?

I don't know, I have been waiting for proper closures in mainstream languages for about 15 years :)

> With effectful helpers like Scoped, I think it's best to be very explicit
> about when the effect is applied.  For example, as is, 'Scoped<T> s = t;'
> compiles and technically means 'Scoped<T> s = Scoped<T>(t);' (construct
> temporary, copy temporary, destroy temporary) and it is only the
> widely-implemented semantics-altering copy-ellision optimization allowed by
> C++ (en.wikipedia.org/wiki/Copy_elision) that gives you the desired
> 'Scoped<T> s(t);'.  That should scare you :)

I am positively scared :)

IIRC, MOZ_GUARD_* should at least produce a clean error if we make this kind of mistake. But yes, obtaining a compile-time error is definitely nicer.

>To this end, could you:
>  - slap 'explicit' on all the current constructors
done

>  - define the copy constructor with 'explicit' (as is, it is implicitly
> defined)
Defined |explicit|, |MOZ_DELETE| and |private|. This should be sufficient :)

>  - MOZ_DELETE operator= and define some equivalent named, e.g., 'reset'
Made |operator=(Scoped&)| |MOZ_DELETE| and |private|. Added |reset|. However, once we have done all of this, I think that we can keep |operator=(T&)| public and usable. Am I right?

Comment 27

[Luke Wagner [:luke]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #26)
> However, once we have done all of this, I think that we can keep
> |operator=(T&)| public and usable. Am I right?

Yes.

Comment 28

[Jeff Walden [:Waldo]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #26)
> > That's hot.  How long until we can depend on lambda compiler support?
> 
> I don't know, I have been waiting for proper closures in mainstream
> languages for about 15 years :)

The clang people are working on lambda support now, so I think you're going to be waiting until the oldest clang dependency is 3.1, at best.  So don't hold your breath.  :-)

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 599125 [details] [diff] [review]
Rudimentary RAII helper

Review of attachment 599125 [details] [diff] [review]:
-----------------------------------------------------------------

Since everyone else seems happy with this, I'll make some comments here.

I'm somewhat unclear what exactly the distinction is we're trying to make with Auto* classes versus Scoped* classes, and why exactly we want to make it.  Could someone concisely explain it so there's a rationale to point to in the future?

::: mfbt/Scoped.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_Scoped_h_

This needs a descriptive comment like the ones the other headers have at the top of them, explaining what the file does so MXR will show a description.

Given that bug 717196 exists, I'd prefer if you just used the old tri-license header, so that that comment will display properly.  :-\  Or maybe MXR can figure out the difference anyway, somehow, if we're lucky.  That'd be best, of course, but I'm not optimistic.

@@ +6,5 @@
> +#define mozilla_Scoped_h_
> +
> +#include "mozilla/GuardObjects.h"
> +
> +/**

Thus far mfbt has followed JS commenting style, which doesn't use /** in comments, for better or for worse.

@@ +9,5 @@
> +
> +/**
> + * Scoped is a helper to create RAII wrappers
> + * The Traits class is expected to look like the following:
> + *   struct Traits {

Blank line between prose and code block in comments.

@@ +25,5 @@
> +public:
> +  typedef typename Traits::type T;
> +
> +  explicit Scoped(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM):
> +  value(Traits::None()) {

explicit Scoped(MGONOP)
  : value(Traits::none())
{
  ...

Same for the other constructors here.

@@ +28,5 @@
> +  explicit Scoped(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM):
> +  value(Traits::None()) {
> +    MOZ_GUARD_OBJECT_NOTIFIER_INIT;
> +  }
> +  explicit Scoped(const T& value MOZ_GUARD_OBJECT_NOTIFIER_PARAM):

In JS we've taken, in this case, to putting the trailing macro on an entirely separate line, like so:

explicit Scoped(const T& value
                MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

I tend to think this is more readable than cramming it onto a line with something users are actually supposed to care about.  Mind doing that?

@@ +35,5 @@
> +  }
> +  ~Scoped() {
> +    if (value != Traits::None()) {
> +      Traits::release(value);
> +    }

mfbt, like JS, doesn't brace single-line if-blocks or else-blocks if the conditional is a single line and all if/else-blocks would take up a single line.

@@ +140,5 @@
> +  typedef int type;
> +  static int None() { return -1; }
> +  static void release(int fd) { close(fd); }
> +};
> +typedef Scoped<ScopedCloseFDTraits> ScopedCloseFD;

I think we want to have the various specializations of the base class in different headers.  File-descriptor utilities in particular should go in something like FileUtils.h or something.  One concern per header and all that.  I don't know where the free/delete helpers should go offhand.

Comment 30

[Nathan Froyd [:froydnj]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #28)
> The clang people are working on lambda support now, so I think you're going
> to be waiting until the oldest clang dependency is 3.1, at best.  So don't
> hold your breath.  :-)

FWIW, GCC already supports lambdas in 4.6.

Comment 31

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #29)
> Comment on attachment 599125 [details] [diff] [review]
> Rudimentary RAII helper
> 
> Review of attachment 599125 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since everyone else seems happy with this, I'll make some comments here.
> 
> I'm somewhat unclear what exactly the distinction is we're trying to make
> with Auto* classes versus Scoped* classes, and why exactly we want to make
> it.  Could someone concisely explain it so there's a rationale to point to
> in the future?

AFAICT, it's just STL vocabulary vs. Boost vocabulary. I am willing to go with either, but I think that Scoped* is slightly clearer than Auto*: "Scoped" does mean that the class is somehow related to scope, while "Auto" only means that it is supposed do do something.

> I think we want to have the various specializations of the base class in
> different headers.  File-descriptor utilities in particular should go in
> something like FileUtils.h or something.  One concern per header and all
> that.  I don't know where the free/delete helpers should go offhand.

I have just removed the file descriptor part, but kept the free/delete helpers, as they are extremely generic.

I will upload the changes once they have returned from TryServer.

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Ok, this seems to work.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

By the way, I have adapted AutoFDClose, etc - see bug 728171.

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 599253 [details] [diff] [review]
Rudimentary RAII helper

># HG changeset patch
># User David Rajchenbach-Teller <[e-mail]>
># Date 1329849341 -3600
># Node ID 8a499263fc4ad6a52169128d4a98bfe9473bb8cd
># Parent  0474267d5bd17dd1685945bdbaa5062f8c04da9c
>* * *
>try: -b do -p all -u xpcshell -t none
>* * *
>try: -b do -p win32 -u xpcshell -t none
>* * *
>try: -b do -p win32 -u xpcshell -t none
>* * *
>try: -b do -p all -u all -t none

Don't forget to remove those

>--- /dev/null
>+++ b/mfbt/Scoped.h
>@@ -0,0 +1,253 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.

"the Mozilla Foundation."; MoCo doesn't own copyright.

>+ * Contributor(s):
>+ *  Mike Hommey <[e-mail]>
>+ *  David Rajchenbach-Teller <[e-mail]>

Indent one more space.

>+#ifndef mozilla_Scoped_h_
>+#define mozilla_Scoped_h_

No trailing underscore.

>+#include "mozilla/GuardObjects.h"
>+#include "mozilla/Attributes.h"

Sort includes.

Comment 35

[(dormant account)]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #14)

> >+  const T& get() const { return value; }
> >+
> 
> non-const too

I strongly disagree with this. .get() typically means you can look at my pointer. 

I subclasses like ScopedDeletePtr should add a .ptr() for rw access.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Small update. 1/ as |operator=(const T&)| felt like a contradiction (we are going to deallocate that |T|, so it is hardly a |const|) 2/ Fixed a typo in |dispose()|. Shockingly, neither gcc nor vc++ realized that I was calling a one-argument function without any argument.

Comment 37

[David Teller [:Yoric] - still alive but not very active]

(In reply to Taras Glek (:taras) from comment #35)
> I strongly disagree with this. .get() typically means you can look at my
> pointer. 
> 
> I subclasses like ScopedDeletePtr should add a .ptr() for rw access.

I agree that this is much clearer.

Comment 38

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 600993 [details] [diff] [review]
Rudimentary RAII helper

>diff --git a/mfbt/Scoped.h b/mfbt/Scoped.h

>+class Scoped

I had misgivings about the name "Scoped" initially but it's grown on me.

Comments look addressed, r=me.

Comment 39

[Luke Wagner [:luke]]

Comment on attachment 600993 [details] [diff] [review]
Rudimentary RAII helper

Thanks for responding to all these comments and sticking with it :)

I'm not thrilled about the macrology, but I guess whenever we can depend on C++11 we can nuke the macros and replace all the uses with alias templates.

Comment 40

[Jeff Walden [:Waldo]]

Comment on attachment 600993 [details] [diff] [review]
Rudimentary RAII helper

Review of attachment 600993 [details] [diff] [review]:
-----------------------------------------------------------------

I seem to remember you wanted to use this in the JS engine as well.  Note that most JS allocations use js_malloc and js_free, so the derived classes you define here aren't suitable for use in the JS engine, most places.

::: mfbt/Scoped.h
@@ +33,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

Apparently we're required to use the MPL2 header now, so use that.

Even tho bug 717196 isn't fixed, add a one-liner summary comment here.  This should work (and then remove it from the comment below):

/* A number of structures to simplify scope-based RAII management. */

@@ +45,5 @@
> + *
> + * Resource Acquisition Is Initialization is a programming idiom used to
> + * write robust code that is able to deallocate resources properly, even
> + * in presence of execution errors or exceptions that need to be propagated.
> + * By definition, the Scoped* classes defined in this header perform

"By definition" is superfluous, please remove it.

@@ +50,5 @@
> + * the deallocation of the resource they hold once program execution reaches
> + * the end of the scope for which they have been defined.
> + *
> + * This header provides the following RAII classes:
> + * - |ScopedFreePtr| - a container for a pointer, that automatically calls

Blank line before this list.

@@ +67,5 @@
> + *
> + * Note that the RAII classes defined in this header do _not_ perform any form
> + * of reference-counting or garbage-collection. These classes have exactly two
> + * behaviors:
> + * - if |forget()| has not been called, the resource is always deallocated at

Blank line before this list.

@@ +82,5 @@
> + * reaching the end of the scope, graphics contexts, etc.
> + */
> +
> +#include "mozilla/GuardObjects.h"
> +#include "mozilla/Attributes.h"

Alphabetize.

@@ +105,5 @@
> +  typedef typename Traits::type T;
> +
> +  explicit Scoped(MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM)
> +    : value(Traits::empty()) {
> +    MOZ_GUARD_OBJECT_NOTIFIER_INIT;

Put the { on a new line so : and the body don't blend together -- also other places in the patch.

@@ +189,5 @@
> + * for more details.
> + */
> +#define SCOPED_TEMPLATE(name, Traits)                          \
> +template <typename Type>                                       \
> +struct name: public Scoped<Traits<Type> >                      \

Space before : when deriving from a class -- and below a couple places, too.

@@ +219,5 @@
> +struct ScopedFreePtrTraits
> +{
> +  typedef T *type;
> +  static T *empty() { return NULL; }
> +  static void release(T *ptr) { free(ptr); }

T* here, not T * (and below a few places too).

Comment 41

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #40)
> Comment on attachment 600993 [details] [diff] [review]
> Rudimentary RAII helper
> 
> Review of attachment 600993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I seem to remember you wanted to use this in the JS engine as well.  Note
> that most JS allocations use js_malloc and js_free, so the derived classes
> you define here aren't suitable for use in the JS engine, most places.

I had the impression that using malloc/free in some circumstances was permitted in the JS engine. Am I wrong?

I also have a prototype trait that captures the context in the pointer to allow using JS_free. This works only if the context has not been destroyed in the scope of the pointer, but I think that this is the general (only?) use case.

> > +#include "mozilla/GuardObjects.h"
> > +#include "mozilla/Attributes.h"
> 
> Alphabetize.

I beg your pardon?

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Attachment: Example usage in JS context

Comment 43

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

A comes before G, so

#include "mozilla/Attributes.h"
#include "mozilla/GuardObjects.h"

Comment 44

[David Teller [:Yoric] - still alive but not very active]

(In reply to Ms2ger from comment #43)
> A comes before G, so
> 
> #include "mozilla/Attributes.h"
> #include "mozilla/GuardObjects.h"

Ah, thanks for the translation.

Comment 45

[David Teller [:Yoric] - still alive but not very active]

Attachment: Rudimentary RAII helper

Here it is, with Waldo's latest remarks.

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Attachment: Variant of the RAII helper, without non-const get()

I attach a variant of the RAII help, in which non-|const| |get()| and non-|const| |operator->| have been replaced with a non-const method |ptr()|, as per Taras' suggestions. See bug 728171 for the impact on client code (which seems basically non-existent). Which one do you think is best?

Comment 47

[Mike Hommey [:glandium]]

I have a nit:

> typedef typename Super::T Pointer;

Super::T is not necessarily a pointer (though for the included templates, it is)

Comment 48

[David Teller [:Yoric] - still alive but not very active]

I think it still conveys the meaning. I could rename it to |Handle| or |Descriptor| if necessary.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 601917 [details] [diff] [review]
Variant of the RAII helper, without non-const get()

Hm ... this seems to have a non-const |get()| and no |ptr()| method.  Wrong patch?

I'm fine with the previous patch, so you don't need to re-request review from me.

Comment 50

[Jeff Walden [:Waldo]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #41)
> I had the impression that using malloc/free in some circumstances was
> permitted in the JS engine. Am I wrong?

"It's complicated."

> I also have a prototype trait that captures the context in the pointer to
> allow using JS_free. This works only if the context has not been destroyed
> in the scope of the pointer, but I think that this is the general (only?)
> use case.

Probably so, yes.  (We used to have a lot of RAII structures that stored a context like this, but I think incremental GC has been removing many of them.)

Comment 51

[Jeff Walden [:Waldo]]

Comment on attachment 601917 [details] [diff] [review]
Variant of the RAII helper, without non-const get()

Review of attachment 601917 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Scoped.h
@@ +11,5 @@
> + * Resource Acquisition Is Initialization is a programming idiom used to
> + * write robust code that is able to deallocate resources properly, even
> + * in presence of execution errors or exceptions that need to be propagated.
> + * The Scoped* classes defined in this header perform
> + * the deallocation of the resource they hold once program execution reaches

This should wrap just like all the other text.

Comment 52

[Jeff Walden [:Waldo]]

Comment on attachment 601910 [details] [diff] [review]
Example usage in JS context

Review of attachment 601910 [details] [diff] [review]:
-----------------------------------------------------------------

ctypes is in this kind of weird fish-nor-fowl position.  It's in the JS engine as far as code location goes.  But it mostly acts as though it were external to the JS engine.  So in this code, plain old malloc and free are probably acceptable, and we don't really need to change this.  :-\

Comment 53

[David Teller [:Yoric] - still alive but not very active]

Attachment: Variant of the RAII helper, without non-const get()

Comment 54

[David Teller [:Yoric] - still alive but not very active]

Attachment: Variant of the RAII helper, without non-const get()

Comment 55

[David Teller [:Yoric] - still alive but not very active]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #49)
> Comment on attachment 601917 [details] [diff] [review]
> Variant of the RAII helper, without non-const get()
> 
> Hm ... this seems to have a non-const |get()| and no |ptr()| method.  Wrong
> patch?

My bad. 

> I'm fine with the previous patch, so you don't need to re-request review
> from me.

Does this mean I have your ok on |ptr()|?

Also, addressed Waldo's remark on comment pagination.

Comment 56

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm ambivalent, so yes :).

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Attachment: RAII helper without non-const |get|

Comment 58

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 603722 [details] [diff] [review]
Variant of the RAII helper, without non-const get()

Waldo, Luke, there was a minor snafu on the variant patch I r?-ed to you. Could you please review this fixed version? It is basically a two-lines patch (plus a few lines of doc).

Comment 59

[Luke Wagner [:luke]]

Comment on attachment 603722 [details] [diff] [review]
Variant of the RAII helper, without non-const get()

I hate to be bikesheddy, but a |.ptr()| method that returns a reference seems strange.  I'm actually used to seeing what you have without this patch, so I'd be happy to leave it as is.  If you or Taras is really dead-set on having it be lexically apparent when a caller is getting rw access, I guess I'd expect something of the form:

  T *ptr();
  const T *ptr() const;
  T &ref();
  const T &ref() const;

Comment 60

[David Teller [:Yoric] - still alive but not very active]

(In reply to Luke Wagner [:luke] from comment #59)
> Comment on attachment 603722 [details] [diff] [review]
> Variant of the RAII helper, without non-const get()
> 
> I hate to be bikesheddy, but a |.ptr()| method that returns a reference
> seems strange.  I'm actually used to seeing what you have without this
> patch, so I'd be happy to leave it as is.  If you or Taras is really
> dead-set on having it be lexically apparent when a caller is getting rw
> access, I guess I'd expect something of the form:
> 
>   T *ptr();
>   const T *ptr() const;
>   T &ref();
>   const T &ref() const;

Are you sure? Recall that, in most cases, T is actually a pointer. So this would return a pointer to a pointer, which may be disconcerting.

Also, having |T& ref()| is somewhat in contradiction with making rw access lexically apparent, so I suppose that you did not intend to add that line.

Comment 61

[Luke Wagner [:luke]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #60)
> Are you sure? Recall that, in most cases, T is actually a pointer. So this
> would return a pointer to a pointer, which may be disconcerting.
> Also, having |T& ref()| is somewhat in contradiction with making rw access
> lexically apparent, so I suppose that you did not intend to add that line.

I was thinking "ref" as in "reference" as in C++ reference, where the value type is T.  I guess it is a contradiction when you assume T = U* for some U?  Given that T is also a file descriptor in one of the uses I saw earlier, it seems like, for the purpose of naming, we should just treat T as an opaque value type.  I tried to think of an unambiguous name that didn't invoke 'reference' or 'pointer', but nothing attractive came to mind.

Again, sorry to nit; this is a problem with making hyper-generic structures that don't really do anything.  On the bright side, we haven't had a Alexandrescu-esque trait-splosion (yet) ;-)

Comment 62

[David Teller [:Yoric] - still alive but not very active]

(In reply to Luke Wagner [:luke] from comment #61)
Then what about
  |T *rwget()|
instead of
  |T *ptr()|
?

Comment 63

[Luke Wagner [:luke]]

rwget() sounds good to me

Comment 64

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #61)
> Again, sorry to nit; this is a problem with making hyper-generic structures
> that don't really do anything.  On the bright side, we haven't had a
> Alexandrescu-esque trait-splosion (yet) ;-)

Just noting in passing, but this gets at what I was thinking when I wrote comment 9, more clearly than I expressed it at the time.  Looking at the r? now.

Comment 65

[Jeff Walden [:Waldo]]

Comment on attachment 603722 [details] [diff] [review]
Variant of the RAII helper, without non-const get()

Review of attachment 603722 [details] [diff] [review]:
-----------------------------------------------------------------

I'm also pretty bleh on ptr for something that's not necessarily a pointer.  rwget() leaves me about as bleh.  I don't have any better ideas.  I'll just point to my previous comment, agreeing with Luke, and wonder if perhaps there's something better than having to bikeshed names here.  Feedback from others appreciated.

Comment 66

[David Teller [:Yoric] - still alive but not very active]

I personally see the following reasons to adopt this very generic code:
- it seems basically ready to land;
- there are already several dependent bugs;
- we even have a few examples (bug 728171);
- it replaces code that is just as generic in /mozglue/linker/Utils.h;
- by opposition to said generic code, it work with VC++;
- the refactoring undertaken as part of bug 728171 suggests that the resulting code is slightly clearer;
- it is concise and generic.

And the following reasons to not adopt it:
- it is [possibly overly] generic.

I personally like generic stuff, but not religiously so. On the other hand, I want my scoped pointers, so I would like to land this (or some replacement), just to be able to get on with my dependent bugs without having to keep looking over my shoulder for API changes in ancestor bugs. 

Any other feedback?

Comment 67

[Luke Wagner [:luke]]

Comment on attachment 603722 [details] [diff] [review]
Variant of the RAII helper, without non-const get()

Oops, I'm not opposed to this landing, I forgot to r+ in comment 63 :)  Sorry about that.

Comment 68

[Jason Orendorff [:jorendorff]]

Comment on attachment 605703 [details] [diff] [review]
RAII helper without non-const |get|

Review of attachment 605703 [details] [diff] [review]:
-----------------------------------------------------------------

Please feel free to disregard all the following bikeshedding.

The name "Scoped" is awfully vague. "Resource" would be more precise and suggests RIAA. "AutoRelease" is better -- it tells exactly what the class does that's useful.

::: mfbt/Scoped.h
@@ +83,5 @@
> +  }
> +  ~Scoped() {
> +    if (value != Traits::empty())
> +      Traits::release(value);
> +  }

It would be a more generic API to just call Traits::release(value), and require that function to check for empty() if necessary. Then ValueType would just have to be copy-constructible and assignable; we wouldn't require operator!= as well.

As it happens, you can pass NULL right through to free/delete/delete[]; the standard specifies that they do nothing in that case. So the extra branch just goes away.

@@ +89,5 @@
> +  operator const T&() const { return value; }
> +  const T& operator->() const { return value; }
> +  T& operator->() { return value; }
> +  const T& get() const { return value; }
> +  T& get() { return value; }

It isn't clear to me from the discussion here what we concluded about these, but I think the non-const ones:
  T& operator->() { return value; }
  T& get() { return value; }
should definitely be removed.

@@ +98,5 @@
> +   * Once |forget| has been called, the |Scoped| is neutralized, i.e. it will
> +   * have no effect at destruction (unless it is assigned another pointer by
> +   * |operator=|).
> +   *
> +   * @return The original pointer.

This comment says "pointer", but the value isn't necessarily a pointer.

@@ +114,5 @@
> +   */
> +  void dispose() {
> +    if (value != Traits::empty())
> +      Traits::release(value);
> +  }

Shouldn't this assign:
  value = Traits::empty();
to avoid calling release() again when the destructor runs?

@@ +131,5 @@
> +   */
> +  Scoped<Traits>& operator=(T& other) {
> +    return reset(other);
> +  }
> +  Scoped<Traits>& reset(T& other) {

I think the argument type in both cases should be 'const T&'.

@@ +159,5 @@
> +#define SCOPED_TEMPLATE(name, Traits)                          \
> +template <typename Type>                                       \
> +struct name : public Scoped<Traits<Type> >                     \
> +{                                                              \
> +  typedef Scoped<Traits<Type> > Super;                         \

The lack of template typedefs in C++ strikes again. :-\

Still, I think this can be a little shorter and nicer with the use of a Curiously Recurring Template. I don't think the traits classes really help, so I would toss those overboard:

template <class MostDerived>
class Scoped
{
public:
  typedef typename MostDerived::ValueType ValueType;

  explicit Scoped(const ValueType &value
                  MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM)
    : value(value)
  {
    MOZ_GUARD_OBJECT_NOTIFIER_INIT
  }

  ~Scoped() { MostDerived::release(value); }

  MostDerived& operator=(const ValueType &v) {
    MostDerived::release(value);
    value = v;
    return *static_cast<MostDerived *>(this);
  }

  void dispose() {
    MostDerived::release(value);
    value = MostDerived::empty();
  }

  ...

private:
  ValueType value;
  MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER
};

#define MOZ_SCOPED_DERIVED_CLASS_CTOR(name) \
  explicit name(ValueType value = empty() \
                MOZ_GUARD_OBJECT_NOTIFIER_PARAM) \
    : Scoped(value \
             MOZ_GUARD_OBJECT_NOTIFIER_ONLY_PARAM_TO_PARENT) \
  {}

template <class T>
class AutoFreePtr : public Scoped<AutoFreePtr<T> >
{
public:
  typedef T* ValueType;
  static ValueType empty() { return NULL; }
  static void release(ValueType ptr) { free(ptr); }
  MOZ_SCOPED_DERIVED_CLASS_CTOR(AutoFreePtr)
};

Hope this makes sense...

Comment 69

[Jason Orendorff [:jorendorff]]

Comment on attachment 601910 [details] [diff] [review]
Example usage in JS context

>+  ScopedJSFreePtr<void> cargs(JSPtr<void>::make(cx, sizeArg));

Note that there's already AutoReleasePtr in jscntxt.h for this.

Comment 70

[Luke Wagner [:luke]]

With great genericity comes great bikeshedding :)

Comment 71

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #69)
> Comment on attachment 601910 [details] [diff] [review]
> Example usage in JS context
> 
> >+  ScopedJSFreePtr<void> cargs(JSPtr<void>::make(cx, sizeArg));
> 
> Note that there's already AutoReleasePtr in jscntxt.h for this.

Ah, good. Of course, if we place Scoped.h in mfbt, we will certainly want to rewrite AutoReleasePtr as an instance of Scoped at some point.

(In reply to Luke Wagner [:luke] from comment #70)
> With great genericity comes great bikeshedding :)

You can laugh, you're not the one with bugs depending on bugs depending on this :)

(In reply to Jason Orendorff [:jorendorff] from comment #68)
> Comment on attachment 605703 [details] [diff] [review]
> RAII helper without non-const |get|
> 
> Review of attachment 605703 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please feel free to disregard all the following bikeshedding.
> 
> The name "Scoped" is awfully vague. "Resource" would be more precise and
> suggests RIAA. "AutoRelease" is better -- it tells exactly what the class
> does that's useful.

Well, |ScopedPtr| sounds much more precise than |AutoReleasePtr| to me. The first one mentions clearly that the only criterium is scope, while the second could involve garbage-collection or reference-counting, for instance.


> ::: mfbt/Scoped.h
> @@ +83,5 @@
> > +  }
> > +  ~Scoped() {
> > +    if (value != Traits::empty())
> > +      Traits::release(value);
> > +  }
> 
> It would be a more generic API to just call Traits::release(value), and
> require that function to check for empty() if necessary. Then ValueType
> would just have to be copy-constructible and assignable; we wouldn't require
> operator!= as well.

Good idea.

> @@ +89,5 @@
> > +  operator const T&() const { return value; }
> > +  const T& operator->() const { return value; }
> > +  T& operator->() { return value; }
> > +  const T& get() const { return value; }
> > +  T& get() { return value; }
> 
> It isn't clear to me from the discussion here what we concluded about these,
> but I think the non-const ones:
>   T& operator->() { return value; }
>   T& get() { return value; }
> should definitely be removed.

Yes, this seems to be the general agreement.

> @@ +114,5 @@
> > +   */
> > +  void dispose() {
> > +    if (value != Traits::empty())
> > +      Traits::release(value);
> > +  }
> 
> Shouldn't this assign:
>   value = Traits::empty();
> to avoid calling release() again when the destructor runs?
> 
> @@ +131,5 @@
> > +   */
> > +  Scoped<Traits>& operator=(T& other) {
> > +    return reset(other);
> > +  }
> > +  Scoped<Traits>& reset(T& other) {
> 
> I think the argument type in both cases should be 'const T&'.

I don't think so: we are going to release this |T| eventually.

> 
> @@ +159,5 @@
> > +#define SCOPED_TEMPLATE(name, Traits)                          \
> > +template <typename Type>                                       \
> > +struct name : public Scoped<Traits<Type> >                     \
> > +{                                                              \
> > +  typedef Scoped<Traits<Type> > Super;                         \
> 
> The lack of template typedefs in C++ strikes again. :-\

That and a alpha-conversion bug in VC++ that forced me to rewrite the macro with more distinct type names.

> 
> Still, I think this can be a little shorter and nicer with the use of a
> Curiously Recurring Template. I don't think the traits classes really help,
> so I would toss those overboard:
> [..]
> Hope this makes sense...

It is midnight here, so it doesn't, but I hope it will on Monday :)

Comment 72

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #68)
> Hope this makes sense...

Ok, think I got it. Thanks for this pattern, I did not know about it.

Also, what gain should we expect from this transformation? The number of lines seems the same, readability seems mostly equivalent (some will certainly prefer the Trait and others the Curiously Recurring Template), and I have the impression that there is no compile-time or runtime speed difference.

Does anyone have a good argument in favor of one of the implementations or the other?

Comment 73

[Jason Orendorff [:jorendorff]]

(In reply to Luke Wagner [:luke] from comment #70)
> With great genericity comes great bikeshedding :)

Touché.

Comment 74

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #71)
> > @@ +114,5 @@
> > > +   */
> > > +  void dispose() {
> > > +    if (value != Traits::empty())
> > > +      Traits::release(value);
> > > +  }
> > 
> > Shouldn't this assign:
> >   value = Traits::empty();
> > to avoid calling release() again when the destructor runs?

I want to point this out one more time since it looked like you might have missed it. It still looks like a bug to me.

> > @@ +131,5 @@
> > > +   */
> > > +  Scoped<Traits>& operator=(T& other) {
> > > +    return reset(other);
> > > +  }
> > > +  Scoped<Traits>& reset(T& other) {
> > 
> > I think the argument type in both cases should be 'const T&'.
> 
> I don't think so: we are going to release this |T| eventually.

We don't release the T referred to by the argument |other|. We'll release |this->value|, which is non-const and is populated by copying |other|.

Comment 75

[David Teller [:Yoric] - still alive but not very active]

Attachment: RAII helper without non-const |get|

Comment 76

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #74)
> > > Shouldn't this assign:
> > >   value = Traits::empty();
> > > to avoid calling release() again when the destructor runs?
> 
> I want to point this out one more time since it looked like you might have
> missed it. It still looks like a bug to me.

Definitely.
Sorry about not answering that specific point, you were so right that I didn't even think to nod :)


> > > @@ +131,5 @@
> > > > +   */
> > > > +  Scoped<Traits>& operator=(T& other) {
> > > > +    return reset(other);
> > > > +  }
> > > > +  Scoped<Traits>& reset(T& other) {
> > > 
> > > I think the argument type in both cases should be 'const T&'.
> > 
> > I don't think so: we are going to release this |T| eventually.
> 
> We don't release the T referred to by the argument |other|. We'll release
> |this->value|, which is non-const and is populated by copying |other|.

Certainly, but whether we release the pointer or a copy of the pointer (or the file descriptor), we are still releasing the same memory (or entry in the table of file descriptors, etc). I believe that marking this |const| would send the wrong message.

Comment 77

[David Teller [:Yoric] - still alive but not very active]

(In reply to Jason Orendorff [:jorendorff] from comment #68)
> Still, I think this can be a little shorter and nicer with the use of a
> Curiously Recurring Template. I don't think the traits classes really help,
> so I would toss those overboard:
> [...]
> Hope this makes sense...

So far, I have been unable to get this code to compile and my short experience with this pattern is rather negative. It looks like that clients of Scoped.h have to provide code that is harder to read, write and get right.

So, unless there is a strong objection, I plan to keep the version based on traits.

Moreover, I would like to land the latest version, if there is no other issue.

Comment 78

[Jason Orendorff [:jorendorff]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #76)
> Certainly, but whether we release the pointer or a copy of the pointer (or
> the file descriptor), we are still releasing the same memory (or entry in
> the table of file descriptors, etc). I believe that marking this |const|
> would send the wrong message.

operator= and reset should have the same argument type as the constructor:

>+  explicit Scoped(const Resource& value
>+                  MOZ_GUARD_OBJECT_NOTIFIER_PARAM)

C++ templates need to be pedantically const-correct or else simple things don't work as expected. For example:

    X makeX();

    Scoped<X> sx(makeX());   // works fine, makeX() has return type X
    sx = makeX();            // ERROR: operator= wants a non-const X
                             // and therefore won't take a temporary

const is definitely what a C++ template uggaprogramming wonk would write here; it's what the compiler wants to see to make the template cooperate nicely with code that uses it. Ping luke or me on IRC if you'd like to talk more--I feel like we're not quite connecting here, perhaps due to limitations of the medium!

(In reply to David Rajchenbach Teller [:Yoric] from comment #77)
> So far, I have been unable to get this code to compile and my short
> experience with this pattern is rather negative. It looks like that clients
> of Scoped.h have to provide code that is harder to read, write and get right.
> 
> So, unless there is a strong objection, I plan to keep the version based on
> traits.

OK. Go strong. I don't mean to derail you.

Comment 79

[David Teller [:Yoric] - still alive but not very active]

Attachment: RAII helper

Bowing to the experience of jorendorff, I adopt the |const| for |operator=| and |reset|.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cc8dc1f19ec

Comment 81

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/5cc8dc1f19ec

Comment 82

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #71)
> (In reply to Jason Orendorff [:jorendorff] from comment #68)
> > The name "Scoped" is awfully vague. "Resource" would be more precise and
> > suggests RIAA. "AutoRelease" is better -- it tells exactly what the class
> > does that's useful.
> 
> Well, |ScopedPtr| sounds much more precise than |AutoReleasePtr| to me. The
> first one mentions clearly that the only criterium is scope, while the
> second could involve garbage-collection or reference-counting, for instance.

We use the term "Auto" all over the place in Mozilla code to indicate classes that are intended to be used on the stack to handle cleanup when they go out of scope. E.g. nsAutoTArray, nsAutoPtr, nsAutoArrayPtr, nsAuto(C)String, gfxContextAuto(Path,Matrix)SaveRestore, AutoBuildingDisplayList, AutoResolveName, AutoEnumStateRooter, AutoSetCurrentTransaction, AutoLock, MonitorAuto(Un)Lock, AutoSetOperator, AutoFILE, AutoResolveFlag, AutoSetInstantiatingToFalse, AutoFallback, AutoCheckOperation, AutoResolveRefLayers, AutoFree, AutoResetInShow,  AutoPrepareForDrawing, AutoPushClipRect ... that's only a partial list, you get the idea.

Was there a really really good reason to use the term "Scoped" instead of "Auto" here?

Comment 83

[David Teller [:Yoric] - still alive but not very active]

Well, "Scoped" means that it is scope-based, while "Auto" just means that something is automatic, without being quite clear as to what is automatic.

Comment 84

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

"Auto" is supposed to mean, "intended to be used in automatic storage", i.e. on the stack.  It should be the case that AutoFoo ==> class NS_STACK_CLASS AutoFoo.

Comment 85

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Right. It seems to me these things are equivalent.

Comment 86

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

No ... I could have a ScopedFoo as a member of a non-NS_STACK_CLASS class.  It's scoped to the enclosing class.  We can't do that with AutoFoo.  (In the model I'm describing.  Reality in gecko is of course much different.)

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I see, that makes some sense.

Unfortunately nsAuto(C)String and nsAutoTArray don't really fit into either Scoped or Auto then ... their purpose is to support "automatic (inline) storage", but they don't necessarily allocate that storage on the stack. (nsAutoTArray in particular is occasionally very useful in heap objects.)

For stuff like AutoBuildingDisplayList it's the lexically-scoped nature of the object, not the automatic storage, that's important ... maybe in an ideal world it would be called ScopedBuildingDisplayList and be NS_STACK_CLASS?