Last Comment Bug 716236 - Cleanup GuardObjects.h
: Cleanup GuardObjects.h
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 07:45 PST by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-01-15 03:19 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (9.80 KB, patch)
2012-01-07 07:45 PST, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review-
Details | Diff | Splinter Review
diff -w (3.26 KB, patch)
2012-01-07 10:30 PST, :Ms2ger (⌚ UTC+1/+2)
no flags Details | Diff | Splinter Review
Patch v2 (10.10 KB, patch)
2012-01-10 07:16 PST, :Ms2ger (⌚ UTC+1/+2)
jwalden+bmo: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-01-07 07:45:10 PST
Created attachment 586671 [details] [diff] [review]
Patch v1

Fixed indentation, added C++ ifdefs, fixed PR_BEGIN/END_MACROs that crept in, and added another comment for MXRs sake.
Comment 1 :Ms2ger (⌚ UTC+1/+2) 2012-01-07 10:30:42 PST
Created attachment 586711 [details] [diff] [review]
diff -w
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-09 15:59:28 PST
Comment on attachment 586671 [details] [diff] [review]
Patch v1

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

All nitpicks, but there are enough I probably should look once more.  I promise review of it tomorrow morning, if you have it by then.

::: mfbt/GuardObjects.h
@@ +37,5 @@
>   * the terms of any one of the MPL, the GPL or the LGPL.
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +/* Implementation of macros to ensure correct use of RAII objects. */

Might be worth making that "RAII Auto* objects", since the term RAII might not necessarily be familiar to everyone.

@@ +44,5 @@
>  #define mozilla_GuardObjects_h
>  
>  #include "mozilla/Assertions.h"
>  
> +#ifdef __cplusplus

What C code do you anticipate validly using this header, even transitively?  I'd prefer seeing this do #ifndef __cplusplus / #error "GuardObjects.h is only usable by C++ code." / #endif or something -- the way Attributes.h did before it got functionality intended for use in both C and C++ added to it.

@@ +50,1 @@
>  namespace mozilla {

I think we don't want people using GuardObject directly -- only through the macros.  JS has taken to putting classes in headers that aren't to be used by user code (at least, not directly) in a detail namespace.  Boost, LibreOffice, and other projects do this, so it's sort of an emerging C++ convention.  How about we do this here, and have mozilla::detail::GuardObject?  It should be about a dozen lines of change when we don't indent the bodies of namespaces.

@@ +50,2 @@
>  namespace mozilla {
> +/**

Single star is more common in mfbt code, for better or worse.

@@ +54,5 @@
> + * when we have a guard object whose only purpose is its constructor and
> + * destructor (and is never otherwise referenced), the intended use
> + * might be:
> + *     AutoRestore savePainting(mIsPainting);
> + * but is is easy to accidentally write:

Let's put blank lines around the example.

@@ +56,5 @@
> + * might be:
> + *     AutoRestore savePainting(mIsPainting);
> + * but is is easy to accidentally write:
> + *     AutoRestore(mIsPainting);
> + * which compiles just fine, but runs the destructor well before the

And here.

@@ +71,5 @@
> + * there).  However, it seems to be true.
> + *
> + * These classes are intended to be used only via the macros immediately
> + * below them:
> + *   MOZILLA_DECL_USE_GUARD_OBJECT_NOTIFIER declares (ifdef DEBUG) a member

Blank line before this.

@@ +98,1 @@
>      bool* mStatementDone;

Omit "private" here, and put a blank line after the declaration.  We're not using the "m" prefix for member variables, either, so just use "statementDone".

@@ +98,4 @@
>      bool* mStatementDone;
> +public:
> +    GuardObjectNotifier()
> +        : mStatementDone(NULL)

Initializing on a single line is consistent with the rest of mfbt, and this isn't long enough for multiple lines to come into question here.

@@ +106,3 @@
>      }
>  
>      void SetStatementDone(bool *aStatementDone) {

* by bool, lowercase the name, and no "a" prefix ("statementDone" and qualify the member reference with this->?).

@@ +112,4 @@
>  
> +class GuardObjectNotificationReceiver
> +{
> +private:

Consistent with JS style, private/public should be half-indented, I think.

@@ +116,1 @@
>      bool mStatementDone;

Blank line after this, and "statementDone" again.

@@ +116,4 @@
>      bool mStatementDone;
> +public:
> +    GuardObjectNotificationReceiver()
> +        : mStatementDone(false)

Single line again.

@@ +130,3 @@
>      }
>  
>      void Init(const GuardObjectNotifier &aNotifier) {

It looks like we use JS-style camelCaps naming for member methods more often than we use InterCaps-named methods, so let's lowercase this while we're here.

@@ +131,5 @@
>  
>      void Init(const GuardObjectNotifier &aNotifier) {
> +        /*
> +         * aNotifier is passed as a const reference so that we can pass a
> +         * temporary, but we really intend it as non-const

Period at the end of a complete sentence.

@@ +134,5 @@
> +         * aNotifier is passed as a const reference so that we can pass a
> +         * temporary, but we really intend it as non-const
> +         */
> +        const_cast<GuardObjectNotifier&>(aNotifier).
> +            SetStatementDone(&mStatementDone);

I think I'd kind of prefer to see a temporary here, rather than have the one statement split across two lines like this.

@@ +141,3 @@
>  
> +#define MOZILLA_DECL_USE_GUARD_OBJECT_NOTIFIER \
> +    mozilla::GuardObjectNotificationReceiver _mCheckNotUsedAsTemporary;

Indentation within defines and such is two spaces, not four.

It's tempting to have one ifdef DEBUG for the class, then a separate ifdef DEBUG / ... / else / ... endif for the macro definitions.  But this works well enough.

@@ +151,5 @@
> +    , const mozilla::GuardObjectNotifier& _notifier
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT \
> +    , _notifier
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_INIT \
> +    do { _mCheckNotUsedAsTemporary.Init(_notifier); } while (0);

Remove the trailing semicolon -- every user supplies it.

@@ +160,5 @@
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_ONLY_PARAM
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM_IN_IMPL
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM_TO_PARENT
> +#define MOZILLA_GUARD_OBJECT_NOTIFIER_INIT do { } while (0);

Remove the trailing semicolon -- every user supplies it.

@@ -157,4 @@
>  
>  #endif /* !defined(DEBUG) */
>  
> -} // namespace mozilla

Some mfbt code uses //, some uses /*, so technically this didn't need to change.  Perhaps we should just always use /**/ for simplicity.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-09 16:02:21 PST
Well, mozilla::detail::whatever, not just GuardObject, if that wasn't clear.
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2012-01-10 00:41:50 PST
(In reply to Jeff Walden (remove +bmo to email) from comment #2)
> @@ +44,5 @@
> >  #define mozilla_GuardObjects_h
> >  
> >  #include "mozilla/Assertions.h"
> >  
> > +#ifdef __cplusplus
> 
> What C code do you anticipate validly using this header, even transitively? 
> I'd prefer seeing this do #ifndef __cplusplus / #error "GuardObjects.h is
> only usable by C++ code." / #endif or something -- the way Attributes.h did
> before it got functionality intended for use in both C and C++ added to it.

jsfriendapi.h is used in C, but I'll ifdef the #include instead.

> @@ -157,4 @@
> >  
> >  #endif /* !defined(DEBUG) */
> >  
> > -} // namespace mozilla
> 
> Some mfbt code uses //, some uses /*, so technically this didn't need to
> change.  Perhaps we should just always use /**/ for simplicity.

I got warnings about // in C, so I'll revert it.

Will fix your other comments.
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-10 01:48:09 PST
(In reply to Ms2ger from comment #4)
> jsfriendapi.h is used in C, but I'll ifdef the #include instead.

Oh, sigh, jsapi.h.  You were right.  Use #ifdef __cplusplus, sorry for the hassle.  I should keep this in mind for anything else that have eventual JSAPI exposure...
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-01-10 07:16:03 PST
Created attachment 587299 [details] [diff] [review]
Patch v2

I kept the private:s, I think it's better to be explicit.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-10 08:27:06 PST
Comment on attachment 587299 [details] [diff] [review]
Patch v2

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

The MOZILLA_->MOZ_ patch landed here, so watch out for the rebase.

::: mfbt/GuardObjects.h
@@ +104,2 @@
>    private:
> +    bool* statementDone;

Fair enough, I guess.  Although given default-privateness being the only difference between classes and structs, I have my doubts anyone will really need it...

@@ +112,3 @@
>      }
>  
> +    void SetStatementDone(bool* statementIsDone) {

setStatementDone

@@ +150,5 @@
> +#endif /* DEBUG */
> +
> +#ifdef DEBUG
> +
> +#define MOZILLA_DECL_USE_GUARD_OBJECT_NOTIFIER \

If you actually are going to have a separate #ifdef DEBUG / defines / #else / empty defines / #endif, please get rid of the blank lines within the two halves, and use "#  define ..." for the definitions.  So you'd have this, then:

#ifdef DEBUG
#  define MOZ_DECL_USE_GUARD_OBJECT_NOTIFIER \
     mozilla::detail::GuardObjectNotificationReceiver _mCheckNotUsedAsTemporary;
#  define MOZ_GUARD_OBJECT_NOTIFIER_PARAM \
     , const mozilla::detail::GuardObjectNotifier& _notifier = \
         mozilla::detail::GuardObjectNotifier()
...

That is, # for all lines is in the initial column, "ifdef" is immediately following, "define" occurs two spaces indented from "ifdef", and a "define"'s body is two spaces further indented from that.  This is consistent with the style used by Assertions.h.
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-01-15 03:19:20 PST
https://hg.mozilla.org/mozilla-central/rev/6701b741568d

Note You need to log in before you can comment on or make changes to this bug.