[haiku-gsoc] Re: [layout API] archiving extension (#6256)
- From: Alex Wilson <yourpalal2@xxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Wed, 14 Jul 2010 21:04:40 -0600
On Wed, Jul 14, 2010 at 6:08 PM, Ingo Weinhold <ingo_weinhold@xxxxxx> wrote:
>
> On 2010-07-13 at 19:03:36 [+0200], Alex Wilson <yourpalal2@xxxxxxxxx> wrote:
>> I've addressed (I think) all the issues we've talked about and
>> attached a new patch to #6256. It includes the ownership stuff,
>> template stuff, etc. I also included my updates to BLayout and
>> BLayoutItem archiving, because they now use the FindObject methods, as
>> FindArchivable no longer exists. The updates there also follow the new
>> method of layout archiving we had discussed.
>>
>> I noticed when including binary_compatibility/Support.h &
>> binary_compatibility/Interface.h that there was errors about
>> redefinitions, and so I've fixed the header guard in
>> binary_compatibility/Global.h.
>>
>> I hope that the patch follows the style guidelines, I did try to at
>> least :P I wasn't totally sure about the style for template method
>> declarations, but I looked around the source, and I think I've used
>> the correct style.
>
> I'm not sure we even have a well-defined style for that. In the patch the
> template methods look OK to me at least, so if we have a style for those,
> they are probably OK. :-)
>
>> Sorry this patch is so big, I had to update the layout classes as I
>> went, and if they weren't included, the build would break.
>
> No worries.
>
> I've started to look through the patch, but I'm already a bit too tired
> tonight, so I'll continue tomorrow. Two things I've seen so far:
>
> * You define NULL_TOKEN in <Archivable.h>. Being a public header, the name
> should be "B_" prefixed. Since B_NULL_TOKEN is already used (internally)
> and since a bit more descriptiveness doesn't harm, I suggest renaming to
> B_ARCHIVABLE_NULL_TOKEN.
>
> * NULL_TOKEN is defined again in ArchivingManagers.cpp.
Sorry, I had originally defined it in Archivable.h, then did a search
on OpenGrok and found B_NULL_TOKEN in TokenSpace.h, so instead I
defined it in Archivable.cpp & ArchivingManagers.cpp, or at least
thats what I meant to do :P. The value of NULL_TOKEN isn't
particularily significant, as any negative value maps to a NULL
pointer, but the '-42' was starting to crop up a little too much, so I
thought it would be more readable to have a #defined value.
> * Regarding the "enum ownership" in <SupportDefs.h>: I'm a bit skeptical of
> namespaceless public type names (who isn't annoyed by name clashes with
> "orientation" or "alignment"?). I suppose suggesting BOwnership will result
> in a storm of protest. Moreover I guess "ownership" is not quite fitting,
> anyway, since the values actually specify ownership operations.
Haha, its funny you should give 'orientation' and 'alignment' enum
types as reasons against just 'enum ownership', as I had figured it
would be okay as it was like those ones :P It can be changed, how
about BecomeParent :P just kidding, hmm, nothing is really coming to
mind as a good name right now, I'll think about it.
--Alex
Other related posts:
