Closed
Bug 1125096
Opened 11 years ago
Closed 11 years ago
Implement JS_DECLARE_NEW_METHODS and JS_DECLARE_MAKE_METHODS using variadic templates
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla38
| Tracking | Status | |
|---|---|---|
| firefox38 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
Details
Attachments
(1 file)
|
17.25 KB,
patch
|
jorendorff
:
review+
|
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.
| Assignee | ||
Comment 1•11 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)
Comment 2•11 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+
| Assignee | ||
Comment 3•11 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
Comment 4•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 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.
Description
•