Closed Bug 1125096 Opened 9 years ago Closed 8 years ago
_DECLARE _NEW _METHODS and JS _DECLARE _MAKE _METHODS using variadic templates
(Reporter: Waldo, Assigned: Waldo)
17.25 KB, patch
|Details | Diff | Splinter Review|
These macros are complete garbage right now, with a gazillion cases repeated almost identically. Happily variadic templates allow us to write a single method and be done with it.
9 years ago
I love how the variadic template syntax forces us to write types with the reference sigil next to the type, as God intended -- not next to the name as is the spawn of Satan syntax. ;-) Also whose idea was it ever that putting the backslashes in macros *right up* against the last character on a line ever preserved readability. :-\ Fixed.
Attachment #8553684 - Flags: review?(jorendorff)
9 years ago
Comment on attachment 8553684 [details] [diff] [review] Patch Review of attachment 8553684 [details] [diff] [review]: ----------------------------------------------------------------- Ahhh. Very nice. ::: js/public/Utility.h @@ +206,5 @@ > * > * Note: Do not add a ; at the end of a use of JS_DECLARE_MAKE_METHODS, > * or the build will break. > */ > #define JS_DECLARE_MAKE_METHODS(MAKENAME, NEWNAME, QUALIFIERS)\ This one is only used in one place (in MallocProvider) and I don't see how there could be a bunch more of them, JS::DeletePolicy being hardcoded in there. So I wouldn't object to deleting this macro, expanding it in the one place where it's used. Your call.
Attachment #8553684 - Flags: review?(jorendorff) → review+
8 years ago
Opted to leave the macro bits, somewhat out of laziness, for now. If we can somehow kill off JS_DECLARE_NEW_METHODS such that *both* are only used/defined inside MallocProvider, at that point I don't mind inlining. But for now, I think you sort of want them both the same, defined the same place, so their definitions evolve (if they evolve) in lockstep, or at least more likely such. https://hg.mozilla.org/integration/mozilla-inbound/rev/0db39f732328
8 years ago
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.