Closed
Bug 977117
Opened 10 years ago
Closed 10 years ago
Inline ObjectIsTypeDescriptor() intrinsic in ion
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: nmatsakis, Assigned: lth)
References
Details
Attachments
(1 file, 1 obsolete file)
18.59 KB,
patch
|
RyanVM
:
checkin+
|
Details | Diff | Splinter Review |
The self-hosted code would like to cheaply test whether a given object is a type descriptor. Lars has been working on a patch in bug 976504.
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → lth
Comment 1•10 years ago
|
||
This has been rewritten with a multi-valued map-across-classes method on TemporaryTypeSet, as discussed on bug 976504.
Attachment #8383049 -
Flags: review?(nmatsakis)
Reporter | ||
Comment 2•10 years ago
|
||
Comment on attachment 8383049 [details] [diff] [review] Patch Review of attachment 8383049 [details] [diff] [review]: ----------------------------------------------------------------- Very nice.j ::: js/src/jsinfer.cpp @@ +1723,5 @@ > + if (true_results) return ForAllResult::MIXED; > + } > + } > + > + return true_results ? ForAllResult::ALL_TRUE : ForAllResult::ALL_FALSE; Maybe add JS_ASSERT((true_results || false_results) && !(true_results && false_results)); ::: js/src/jsinfer.h @@ +674,5 @@ > const Class *getKnownClass(); > > + /* Result returned from forAllClasses */ > + enum ForAllResult { > + EMPTY=1, // Set empty Any particular reason you started this at 1?
Attachment #8383049 -
Flags: review?(nmatsakis) → review+
Comment 3•10 years ago
|
||
(In reply to Niko Matsakis [:nmatsakis] from comment #2) > Comment on attachment 8383049 [details] [diff] [review] > Patch > > Review of attachment 8383049 [details] [diff] [review]: > ----------------------------------------------------------------- > > Very nice.j > > ::: js/src/jsinfer.cpp > @@ +1723,5 @@ > > + if (true_results) return ForAllResult::MIXED; > > + } > > + } > > + > > + return true_results ? ForAllResult::ALL_TRUE : ForAllResult::ALL_FALSE; > > Maybe add JS_ASSERT((true_results || false_results) && !(true_results && > false_results)); Sure, I can do that. > ::: js/src/jsinfer.h > @@ +674,5 @@ > > const Class *getKnownClass(); > > > > + /* Result returned from forAllClasses */ > > + enum ForAllResult { > > + EMPTY=1, // Set empty > > Any particular reason you started this at 1? Yes, though maybe not a good reason from where you're sitting. I'm trying to cultivate a habit where '0' is not used to carry meaning for bitfields and enums: I got burned once because, on Unix, O_RDONLY|O_WRONLY != O_RDWR, even though it is otherwise defined that the O_whatever flags are bits that can be OR'ed together. The underlying problem is that O_RDONLY is defined as 0. This is a known problem that I tracked through available source code back to at least BSD 2.12 (ca 1980), at which point fcntl.h had been introduced and was providing these flags (7th Edition did not have the header or the flags). Despite being a known bug for 35 years it is basically not fixable for binary compatibility reasons. We're not dealing with a public API with binary compatibility issues here, but I still think it's a useful habit to cultivate...
Comment 4•10 years ago
|
||
Added the requested assert (in a slightly more compact form), and clarified the commit message in the patch. Otherwise unchanged.
Attachment #8383049 -
Attachment is obsolete: true
Attachment #8383155 -
Flags: checkin?(nmatsakis)
Assignee | ||
Updated•10 years ago
|
Assignee: lth → lhansen
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cb5d636acdf In the future, please just set the checkin-needed bug keyword when your patch is ready to land.
Flags: in-testsuite+
Updated•10 years ago
|
Attachment #8383155 -
Flags: checkin?(nmatsakis) → checkin+
Comment 6•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9cb5d636acdf
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•