Layout API: Make it FragileBaseClass safe

[mmadia]

As mentioned on this ​haiku-development mailing list thread, the Layout API needs to be made FragileBaseClass (aka FBC) safe. That is, adding a whole bunch of padding virtuals and padding data to each class as needed.

[mmadia]

Setting this as a blocker for entering R1/Beta's.

[yourpalal]

Now that BObjectList is public, we might as well migrate BLayout and friends to it as well. I mention this here as a reminder, and that it will need to be done before/as part of stabilizing the ABI.

[yourpalal]

Thinking further, I don't think we have anything to gain by switching to BObjectList in BLayout.

So, that means this ticket boils down to:

• Adding virtual status_t Perform() methods to all relevant classes. Currently this is inherited from BArchivable, but none of the layout classes implement it.

• Filling in any other virtual methods that may need to be overridden at a later date with stubs of the form

  foo
  Bar::Baz()
  {
      return Base::Baz();
  }
  

• Adding virtual method padding (here are my proposed amounts)
  • 5 for BLayoutItem
  • 10 for BLayout (plus the 5 from BLayoutItem)
  • 5 for BTwoDimensionalLayout -> these are inherited by BGroup/BGrid Layout as well, so they probably don't need any of their own
  • BCardLayout/BSplitLayout probably don't need any, I would (mostly) considered these to be leaf classes.
• Adding uint32 padding: Here I think we could give each class about the same amount of uint32's as they get in virtual methods. For leaf classes, I might add 5 uint32's for good measure.
• anything else??

One thing I wonder about is whether it is worth it to modify these numbers a bit to get our structures 8 byte-aligned or something. My intuition says it probably won't matter, but maybe I'm wrong :)

[yourpalal]

I forgot to include:

• BLayoutContext should get some uint32 padding (maybe to carry extra info about the context), no need for virtuals, I would think.

[bonefish]

I'd be a bit more generous with virtual method padding. It's rather cheap, since it's only added to the vtable, not to the objects. So 10 slots for every polymorphic class (including BAbstractLayout[Item], B{Group,Grid}Layout,...) is a good idea, I think. The member padding contributes to the object size, so it should be based on how likely additions seem. I would add at least add 2 uint32s to every public class (except basic ones like BSize and BAlignment), maybe up to 8 or 10.

Alignment of object sizes isn't necessary. I also wouldn't move to BObjectList.

Further tasks:

• Override all virtual methods that one might need to override in the future (including Perform()). They would just call their base class versions, ATM.
• Add private copy constructor and assignment operator declarations (without implementations) where we don't want to allow copying (in most classes I guess).

[yourpalal]

Replying to bonefish:

I'd be a bit more generous with virtual method padding. It's rather cheap, since it's only added to the vtable, not to the objects. So 10 slots for every polymorphic class (including BAbstractLayout[Item], B{Group,Grid}Layout,...) is a good idea, I think.

That sounds reasonable, yeah.

The member padding contributes to the object size, so it should be based on how likely additions seem. I would add at least add 2 uint32s to every public class (except basic ones like BSize and BAlignment), maybe up to 8 or 10.

sure

Further tasks:

• Override all virtual methods that one might need to override in the future (including Perform()). They would just call their base class versions, ATM.

Hey, I already had that one! :)

• Add private copy constructor and assignment operator declarations (without implementations) where we don't want to allow copying (in most classes I guess).

Good addition, thanks for your feedback. :)

[bonefish]

Replying to yourpalal:

Replying to bonefish:

Further tasks:

• Override all virtual methods that one might need to override in the future (including Perform()). They would just call their base class versions, ATM.

Hey, I already had that one! :)

Oops, sorry. I read the comment days ago and only skimmed it again before replying. Not sure how I missed that section. I probably did something else concurrently and wasn't as focused as I should have been. I vaguely recall that I had another item as well, but for the life of me can't remember what that might have been.

At any rate, good initiative. It would be great to finally close that chapter.

[yourpalal]

Replying to bonefish:

Oops, sorry. I read the comment days ago and only skimmed it again before replying. Not sure how I missed that section. I probably did something else concurrently and wasn't as focused as I should have been.

:) That's alright!

I vaguely recall that I had another item as well, but for the life of me can't remember what that might have been.

We once talked about making BLayoutItem inherit virtually from BArchivable (now's the time), maybe that was it? Checking for public members / inline functions accessing public members (I don't think there are any)? new/delete operators (don't think they're needed)?

At any rate, good initiative. It would be great to finally close that chapter.

Thanks :). Yes, I want to get this done so it doesn't hold up b1, and so that I can work on other stuff!

[bonefish]

Replying to yourpalal:

Replying to bonefish:

I vaguely recall that I had another item as well, but for the life of me can't remember what that might have been.

We once talked about making BLayoutItem inherit virtually from BArchivable (now's the time), maybe that was it?

No. I don't quite recall the details of the discussion, but I don't think that will be necessary.

Checking for public members / inline functions accessing public members (I don't think there are any)?

I can't think of any either, but it certainly doesn't harm to check.

new/delete operators (don't think they're needed)?

I wouldn't think so.

[yourpalal]

Fixed in hrev43514 :)

[tqh]

As pointed out by someone on IRC hrev43514 seems to not be about FBC at all. yourpalal is this really fixed?

[tqh]

I found it in commits mail, so it seem to be a problem with the tags perhaps.

[yourpalal]

Replying to tqh:

As pointed out by someone on IRC hrev43514 seems to not be about FBC at all. yourpalal is this really fixed?

Yeah, it is fixed, the problem is that the changeset that was tagged as hrev43514 was actually a merge commit that indeed didn't include any changes related to the layout api. Maybe I should have said 'Merge pushed in hrev43514' Furthermore, the 'real' merge was actually a84e14ca84 (one of the parents of hrev43514), after which there was one more change pushed, which was merged in the changeset tagged as hrev43514.