Closed Bug 801839 Opened 12 years ago Closed 12 years ago

IonMonkey: Clean up jsop_getprop().

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: sstangl, Unassigned)

Details

(Whiteboard: [ion:t])

Attachments

(1 file)

Attached patch patchSplinter Review
I've been complaining about jsop_getprop() for a while. This finally cleans it up.
Attachment #671587 - Flags: review?(nicolas.b.pierron)
Comment on attachment 671587 [details] [diff] [review] patch Review of attachment 671587 [details] [diff] [review]: ----------------------------------------------------------------- Nice refactoring moving complex if statements in well named functions which can easily be commented or sorted differently. The emitted pattern is a bit difficult to read at first but is convenient because we can reuse the abort function for the return value of any sub-functions. The only trouble that need to be addressed during reviews is that no “return true;” is added after any MIR allocation or graph modification. Most of our optimizations phase can be rewritten with this “emitted pattern” instead of using a 3 state enum return, such as inlining of native functions. ::: js/src/ion/IonBuilder.cpp @@ +5801,5 @@ > + > + // Try to hardcode known constants. > + if (!GetPropTryConstant(&emitted, obj, id) || emitted) > + return emitted; > + nit: trailing white space @@ +5833,5 @@ > + return pushTypeBarrier(call, types, barrier); > +} > + > +bool > +IonBuilder::GetPropTryArgumentsLength(bool *emitted) nit: follow the convention used for IonBuilder. The first upper case letter is used for static functions. @@ +5835,5 @@ > + > +bool > +IonBuilder::GetPropTryArgumentsLength(bool *emitted) > +{ > + *emitted = false; nit: emitted should always be false, no need to reset it. Otherwise assert that it is still false. @@ +5843,5 @@ > return abort("Type is not definitely lazy arguments."); > + if (isArguments != DefinitelyArguments) > + return true; > + > + if (JSOp(*pc) == JSOP_LENGTH) { nit: Invert the condition and remove the braces. @@ +5903,4 @@ > > + TypeOracle::Unary unary = oracle->unaryOp(script_, pc); > + TypeOracle::UnaryTypes unaryTypes = oracle->unaryTypes(script_, pc); > + types::StackTypeSet *barrier = oracle->propertyReadBarrier(script_, pc); Make sure that TI is not freezing twice. @@ +5946,2 @@ > if (!TestCommonPropFunc(cx, unaryTypes.inTypes, id, &commonGetter, true, &isDOM)) > + return false; This TestCommonPropFunc if-statement sounds weird or badly named, it sounds like the commonGetter flag behaves in a similar way as the emitted flag. Can you re-use the same pattern for this function. @@ +6036,5 @@ > + // The input value must either be an object, or we should have strong suspicions > + // that it can be safely unboxed to an object. > + if (unary.ival != MIRType_Object && !unaryTypes.inTypes->objectOrSentinel()) > + return true; > + nit: trailing white-space. @@ +6073,3 @@ > > + if (oracle->propertyReadAccessGetter(script_, pc)) > + monitorResult(load, barrier, types); nit: Reuse the variable “accessGetter” in the condition.
Attachment #671587 - Flags: review?(nicolas.b.pierron) → review+
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: