Closed
Bug 716236
Opened 12 years ago
Closed 12 years ago
Cleanup GuardObjects.h
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla12
People
(Reporter: Ms2ger, Assigned: Ms2ger)
Details
Attachments
(2 files, 1 obsolete file)
3.26 KB,
patch
|
Details | Diff | Splinter Review | |
10.10 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
Fixed indentation, added C++ ifdefs, fixed PR_BEGIN/END_MACROs that crept in, and added another comment for MXRs sake.
Attachment #586671 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 1•12 years ago
|
||
Comment 2•12 years ago
|
||
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.
Attachment #586671 -
Flags: review?(jwalden+bmo) → review-
Comment 3•12 years ago
|
||
Well, mozilla::detail::whatever, not just GuardObject, if that wasn't clear.
Assignee | ||
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
(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...
Assignee | ||
Comment 6•12 years ago
|
||
I kept the private:s, I think it's better to be explicit.
Attachment #586671 -
Attachment is obsolete: true
Attachment #587299 -
Flags: review?(jwalden+bmo)
Comment 7•12 years ago
|
||
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.
Attachment #587299 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 8•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6701b741568d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in
before you can comment on or make changes to this bug.
Description
•