On Fri, Oct 1, 2010 at 10:34 PM, Vyacheslav Egorov <[email protected]> wrote: > I agree that it requires more typing and probably a local variable > initializer. > > But I think we should definitely shoot for static checking here > despite their ugliness and verbosity. DEBUG only runtime checks proved > to be insufficient. Also Ugliness&Verbosity is something that will > always catch reviewer's eye.
Well, to be fair, our current DEBUG checks only fire in case we get a failure, which is rare and hard to cover by tests. An assert failing each time someone accesses an object without a prior failure check is significantly more reliable (and only requires hitting a function once in a test which is something we should strive to do anyway). I'm fine with ugliness and verbosity when it makes things more explicit but not when the sheer amount of code may make it easy to overlook something. Actually I don't think we'll get that much extra code with the AsObject approach, but still, additional locals tend to clutter the code. -- Vitaly > On Fri, Oct 1, 2010 at 8:23 PM, Vitaly Repeshko <[email protected]> wrote: >> On Fri, Oct 1, 2010 at 10:15 PM, Vyacheslav Egorov <[email protected]> >> wrote: >>> We are thinking in the similar direction. >>> >>> But we would like to combine check and T* value extraction in a single >>> operation. When I last talked to Erik and Kevin we decided to use >>> something along the lines of >>> >>> T* var; >>> if (!result->AsObject(&var)) return result; >>> >>> This variation of poor man's pattern matching gives us static >>> guarantee instead of dynamic check. >> >> This requires having an extra local variable and I'm not sure all >> compilers will be able to understand that it is initialized after this >> line. You probably still need a dynamic check if you want to prevent >> the result from being ignored. >> >> >> Thanks, >> Vitaly >> >>> On Fri, Oct 1, 2010 at 7:03 PM, Vitaly Repeshko <[email protected]> >>> wrote: >>>> Here's my take on this problem: http://codereview.chromium.org/3558004/show >>>> >>>> There's a new FailureOr<T> template >>>> (http://codereview.chromium.org/3558004/diff/1/8) that is supposed to >>>> be used in allocation functions like this: >>>> >>>> FailureOr<JSObject> AllocateJSObject(Map* map); >>>> >>>> and in consumer functions like this: >>>> >>>> FailureOr<JSObject> object = AllocateJSObject(map); >>>> if (object.is_failure()) return object; // check and propagate. >>>> object->SetProperty(...); // operator-> requires a prior is_failure >>>> check in debug mode. >>>> FooBar(object.get()); // get() returns the object (also requires the >>>> check). >>>> // ~FailureOr<T> requires that the object was either checked or copied. >>>> >>>> In the patch linked above I only converted a few functions (it >>>> compiles and passes the tests). Until we have everything switched to >>>> FailureOr there are a few public helper methods adding unnecessary >>>> ugliness. I too agree that we should eventually get rid of automatic >>>> Failure to Object upcasting. These two types should only meet in >>>> really low-level places like the C++/JS boundary. >>>> >>>> What do you guys think? >>>> >>>> >>>> Thanks, >>>> Vitaly >>>> >>>> On Thu, Sep 30, 2010 at 3:00 PM, Kevin Millikin <[email protected]> >>>> wrote: >>>>> Just thinking out loud: instead of a base class that both Failure and >>>>> Object >>>>> (couldn't it be HeapObject?) extend, you could use two separate channels. >>>>> Change all the allocation functions to take a HeapObject** and return a >>>>> error code. >>>>> Instead of: >>>>> MUST_USE_RESULT static Object* CreateJSValue(JSFunction* constructor, >>>>> Object* value) { >>>>> Object* result; >>>>> ALLOC_CHECK(result =, Heap::AllocateJSObject(constructor)); >>>>> JSValue::cast(result)->set_value(value); >>>>> return result; >>>>> } >>>>> You could have some variant of: >>>>> MUST_USE_RESULT static ErrorCode CreateJSValue(JSFunction* constructor, >>>>> Object* value, >>>>> HeapObject** result) { >>>>> ErrorCode code; >>>>> if (!(code = Heap::AllocateJSObject(constructor, result))) { >>>>> return code; >>>>> } >>>>> JSValue::cast(result)->set_value(value); >>>>> return ErrorCode::Success(); >>>>> } >>>>> It avoids the nasty macro that takes half an assignment expression as >>>>> argument. >>>>> On Thu, Sep 30, 2010 at 3:39 AM, Erik Corry <[email protected]> wrote: >>>>>> >>>>>> http://codereview.chromium.org/3522008 >>>>>> >>>>>> This is an experiment. The edits to objects.cc are done with an >>>>>> automated tool and are intended as an illustration. They probably >>>>>> don't compile. Would this be a win and can anyone think of a neater >>>>>> way to do it? >>>>>> >>>>>> -- >>>>>> Erik Corry, Software Engineer >>>>>> Google Denmark ApS - Frederiksborggade 20B, 1 sal, >>>>>> 1360 København K - Denmark - CVR nr. 28 86 69 84 >>>>>> >>>>>> -- >>>>>> v8-dev mailing list >>>>>> [email protected] >>>>>> http://groups.google.com/group/v8-dev >>>>> >>>>> -- >>>>> v8-dev mailing list >>>>> [email protected] >>>>> http://groups.google.com/group/v8-dev >>>> >>>> -- >>>> v8-dev mailing list >>>> [email protected] >>>> http://groups.google.com/group/v8-dev >>>> >>> >> > -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
