[Bug 30430] Make wocky_implement_finish_* macros safer

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=30430

--- Comment #5 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2010-09-28 11:34:05 PDT ---
(In reply to comment #4)
> Avoiding g_simple_async_report_error_in_idle, just for this, seems to me to
> make our code less clear for very little gain?

Looking at the patch again, I don't see any difference on the code clearness
(please specify what is unclear to you). Actually, it's somewhat better since
at some places we just leak or unref a GSimpleAsyncResult that we had created
for the case there was no error. With that code we use it.

The goal here is to prepare for wocky_implement_finish_*() macro to get moved
to GLib, but this won't happen until we flip the _is_valid() call with the
propagate call. The internal GLib code analyses is wrong approach, you need to
respect the way they where designed to be used.

> I'd prefer to keep all the stuff dealing with source_object together, *then* do
> all the stuff dealing with cancellable (in other words, move the indicated line
> before the "if").

Ok, will do.

> This does change the semantics for the case where cancellable is NULL: if
> output_cancellable isn't already NULL, it's not overwritten. This seems to be
> checked for already, but it might be worth asserting.

Concurrent async calls are generally not allowed in Wocky, so yes we should
generally assert for cancellable and result to be NULL (we usually assert/error
for only one of them). I can give a pass for that, to make sure we are
consistent.

> 
> > 3. The macro is missing scope
> 
> This is secretly several commits...

I apologies, macros are a pain to edit and split properly.

> 
> > * Make the wocky_implement_*() macro safer by calling _is_valid() before
> >   trying to propagate errors.
> 
> As explained above, I'm not sure how useful this actually is.

This is by design the way it should be used, and #1 blocker to get this utility
into GLib.

> 
> > +  GSimpleAsyncResult *__simple; \
> 
> You didn't mention in the commit message that you switched to
> double-underscored variable names. All double-underscored names are reserved
> for use by the compiler/C library, so this is wrong. Use a single underscore if
> you want to avoid collisions with library-user-defined variable names (but to
> be honest, these macros are the entire body of a function, so I don't really
> see what they'd be colliding with).

Aren't Wocky a library ? Honestly I just forgot about this change (was going to
revert it at some point). This change was done while trying to mimic GLib macro
style, which consistently use double __ inside macros. I can move to single _.
I'll move __p to.

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