Closed Bug 977117 Opened 6 years ago Closed 6 years ago

Inline ObjectIsTypeDescriptor() intrinsic in ion

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: nmatsakis, Assigned: lth)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → lth
Blocks: PJS
Attached patch Patch (obsolete) — Splinter Review
This has been rewritten with a multi-valued map-across-classes method on TemporaryTypeSet, as discussed on bug 976504.
Attachment #8383049 - Flags: review?(nmatsakis)
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+
(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...
Attached patch Patch v2Splinter Review
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: lth → lhansen
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+
Attachment #8383155 - Flags: checkin?(nmatsakis) → checkin+
https://hg.mozilla.org/mozilla-central/rev/9cb5d636acdf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 979139
You need to log in before you can comment on or make changes to this bug.