[Webkit-unassigned] [Bug 101079] [GTK] Clean up g_return macros usage in GObject DOM bindings

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=101079





--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-11-08 10:29:01 PST ---
(In reply to comment #3)
> (From update of attachment 172108 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172108&action=review
> 
> Looks mostly good to me, I just have a couple of questions.
> 
> > Source/WebCore/ChangeLog:11
> > +          - Use them only to check parameters of public API.
> 
> Not trying to get into a philosophical debate here, and it's really not so relevant for this bug because I basically agree with the changes, but I don't see this as a good practice per se. The good practice about g_return macros is to use them in public API instead of ASSERTs or no checks at all. The reason for that is that we cannot control what the user gives us, so it makes sense to check things but it does not make sense to crash on error. That being said, using g_return macros internally makes sense for situations where you want to fail, but do not want to crash (ie, for situations that are neither a programmer's error nor so critical as to justify just giving up and crashing the program).

The main reason to not use the g_return macros internally is because they can be disabled (it's not very common, but it possible). If something is never expected to fail, then I think it's better to crash because the program is likely to crash sooner or later anyway, and it helps to debug the issue. If something is expected to fail we can simply return early and avoid the overhead of the g_return macros or a crash if they are disabled. See the glib documentation:

"The g_return family of macros (g_return_if_fail(), g_return_val_if_fail(), g_return_if_reached(), g_return_val_if_reached()) should only be used for programming errors, a typical use case is checking for invalid parameters at the beginning of a public function. They should not be used if you just mean "if (error) return", they should only be used if you mean "if (bug in program) return". The program behavior is generally considered undefined after one of these checks fails. They are not intended for normal control flow, only to give a perhaps-helpful warning before giving up."

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1233
> > +    return request ? request->priv->coreObject.get() : 0;
> 
> This seems consistent with what we do in most places (but not all!) in WebKitGTK+, so I think that's OK.
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1245
> > +    ASSERT(coreObject);
> 
> Any reason why this should be different than ::core or ::kit?

Yes, in core or kit NULL is a valid expected value. For example, a method that returns a core element can return NULL in case of exception, the conversion to a WebKit object in this case is NULL. Here, we never expect NULL for coreObject, it's a construct only property of WebKitDOMObject and the program is going to crash sooner or later, because we are assuming coreObject is a valid pointer everywhere. 

> > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1346
> > +        return 0;
> 
> Same comment than in ::core..

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.