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
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 07:45 PST by :Ms2ger
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
jwalden+bmo: review-
Details | Diff | Review
diff -w (3.26 KB, patch)
2012-01-07 10:30 PST, :Ms2ger
no flags Details | Diff | Review
Patch v2 (10.10 KB, patch)
2012-01-10 07:16 PST, :Ms2ger
jwalden+bmo: review+
Details | Diff | Review

Description :Ms2ger 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 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 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 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.

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