Closed Bug 1125096 Opened 7 years ago Closed 7 years ago

Implement JS_DECLARE_NEW_METHODS and JS_DECLARE_MAKE_METHODS using variadic templates

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(1 file)

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.
Attached patch PatchSplinter Review
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)
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+
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
https://hg.mozilla.org/mozilla-central/rev/0db39f732328
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.