From: Jonathan Gordon <jdgordy_at_gmail.com>
Date: Tue, 8 Nov 2011 00:30:57 +1100

On 8 November 2011 00:16, Thomas Martitz <kugel_at_rockbox.org> wrote:
> Am 06.11.2011 16:24, schrieb Jonathan Gordon:
>>
>> Three macros and a typedef have been added.
>> SKINOFFSETTOPTR() and PTRTOSKINOFFSET() convert between the offset and
>> the real pointer. I originally wanted to put the type in the first
>> macro and use that instead of void* but not sure if that is really
>> needed.
>> the 3rd macro OFFSETTYPE() is added because I realised the structs
>> used by the various tags will become very confusing if all the members
>> are skinoffset (or long) types instead of the pointer type which the
>> data actually contains. So I added this macro to hopefully help
>> document the code a bit.
>
> I'm not a too huge fan of this macros. Perhaps they're just too long :).  I
> particularly don't like OFFSETTYPE(). I got it right that it just for
> documenting the purpose of the field? A comment can achieve the same.
>

Much of a muchness really. If your editor does nice syntax hilighting
it is more obvious than a comment.

> What about making the skin buffer have handle based  api (skin_get_data(int)
> to be added), with the handle being that offset?
>
> What you could also do is perhaps load it with pointers into the plugin
> buffer, then move it into the buflib, and fix up the pointers using the same
> code that a possible move_callback() would call. How does this sound?
>
> Best regards.
>

I've just now finished moving every pointer from
apps/gui/skin_engine/* to using the macros, So I'm going to finish it
and then it is relatively easy to switch. The benefit in using macros
is they are faster/smaller than a function call and do the same thing.

They are a bit wordy but I don't think it really is a big deal.

And no, it is almost impossible to correctly and safely move every
pointer after load which is why I'm storing them everywhere as
offsets.

Just checked, I'm easily half way through the conversion so hopefully
I'll have a testable patch tomorrow. Unfortunately testing is probably
going to mean commiting and praying because we have no way to test
every single tag and its different uses which is what is really
needed.

Jonathan

Received on 2011-11-07