Bug 27772

It's OK.

Only one comment:

6c6ef72d3f6b0139ece26234e3f0547727d2f6e1:
--
log-store-xml: de-obfuscate priv access

This is a little more involved because we have various pointers to the
superclass; consistently calling the class you're writing "self" and its
superclass "store" (or whatever) is a good way out of that.
--

TplLogStore is a GInterface, which I think do not change the patch. Can you update the commit's comment before publishing the branch, please?

(In reply to comment #0)
> The logger's GET_PRIV macros are neither obvious nor type-safe. self->priv (as
> used in telepathy-glib) is (usually) less typing, and considerably safer.
> 
> See the branch:
> 
> http://git.collabora.co.uk/?p=user/smcv/telepathy-logger.git;a=shortlog;h=refs/heads/get-priv-easier

This branch mostly looks good, though I think we should go a step farther and
only deref self (for self->priv) after g_return_if_fail (TP_IS_<TYPE> (self));

Many of the functions either already have this check or it shouldn't matter
(eg, the dispose and finalize functions, and, arguably, any static functions),
but many non-static functions (eg, in tpl-contact.) only perform this check
after dereffing self.

Feel free to commit after fixing those, or if I'm missing something that
guarantees self->priv is always safe given an arbitrary pointer, without
confirming its type (and that it's non-NULL, like the _IS_TYPE() macros do).

answering to Travis'#2:

this amend is already in http://git.collabora.co.uk/?p=user/kalfa/telepathy-logger.git;a=shortlog;h=refs/heads/tpl-channel-refactored

I do not disagree to add it here, but probably it might save a bit of Simon's time. Hopefully tpl-channel-refactored will be reviewed soon.

rethinking, if Simon agrees, I'll cherry-pick the said patches putting them in a separate 'type-safe' branch, which probably would make more sense.

This after merging this branch to master.

A different bug might be filed if there are further suggestion about how to improve the current type safety.

(In reply to comment #2)
> This branch mostly looks good, though I think we should go a step farther and
> only deref self (for self->priv) after g_return_if_fail (TP_IS_<TYPE> (self));

Not a regression - GET_PRIV dereferenced self->priv just as much as my branch does.

I'm appending some patches, though.

(In reply to comment #4)
> rethinking, if Simon agrees, I'll cherry-pick the said patches putting them in
> a separate 'type-safe' branch, which probably would make more sense.
> 
> This after merging this branch to master.
> 
> A different bug might be filed if there are further suggestion about how to
> improve the current type safety.

Would you mind if I just get this branch in? I realise it'll probably cause conflicts for you, but it's mostly quite "mechanical" changes that should be easy to review.

If you have patches that do small, uncontroversial bits of refactoring, it's a good idea to get them fast-tracked in as their own branch (I often use the name "trivia" for such branches), without waiting for any more controversial changes that appear elsewhere.

(In reply to comment #1)
> TplLogStore is a GInterface, which I think do not change the patch. Can you
> update the commit's comment before publishing the branch, please?

Done.

(In reply to comment #2)
> This branch mostly looks good, though I think we should go a step farther and
> only deref self (for self->priv) after g_return_if_fail (TP_IS_<TYPE> (self));

Done, in every case where it seemed to matter, and some that didn't. Re-review please?

(In reply to comment #5)
> (In reply to comment #2)
> > This branch mostly looks good, though I think we should go a step farther and
> > only deref self (for self->priv) after g_return_if_fail (TP_IS_<TYPE> (self));
> 
> Not a regression - GET_PRIV dereferenced self->priv just as much as my branch
> does.
> 
> I'm appending some patches, though.

Right - it wasn't a regression, but we might as well fix those while we're at it.

(In reply to comment #7)
> (In reply to comment #2)
> > This branch mostly looks good, though I think we should go a step farther and
> > only deref self (for self->priv) after g_return_if_fail (TP_IS_<TYPE> (self));
> 
> Done, in every case where it seemed to matter, and some that didn't. Re-review
> please?

Looks good to me.

There are several branches to be reviewed, one is danni's and three are mine, at least.

I agree that they are pretty mechanical and if they get in before your branch, you'll have to do again the same mechanical work you did yesterday, exactly as us for the pending branches.
In the end, since one of us will have to do some work, I'd prefer to have it in.

Probably it would be nice to ask Danielle too. I don't know if Trevis is working on TPL ATM.

Due to one more commit to Simon's branch, i'm reviewing only d84fa9686e619e4329c2b55dfaa07054ae5cff32.

in log_store_xml_get_dir(), log_store_xml_get_all_files() and log_store_xml_get_chats() priv is assigned but actually never used, it can be removed.

in log_store_xml_get_dir() priv is used only once, so either

1) remove the priv declaration and use self->priv instead

or

2) declare "TplLogStoreXmlPriv *priv" in the other functions in which priv appears just once or twice.



would be appropriate IMHO.

or 3) leave it as is :)

i'd be for 1).


The rest is OK.