Re: Bug 34465 - get rid of all calls to virtual const SfxPoolItem* Put( const SfxPoolItem&, USHORT nWhich)

On Thu, Nov 22, 2012 at 08:52:20AM +0100, Maciej Rumianowski wrote:
> I have hit a problem that i don't fully understand. Disabled items are
> SfxVoidItem with Which 0, but inserted with different one. State of Item is
> check with TYPE Macro as below
>
> > if ( (*ppFnd)->Type() == TYPE(SfxVoidItem) )
> >                         return SFX_ITEM_DISABLED;
>
> So it just type that decides about disabled state of item. I've changed
> places where conditions like
>
> > if( !(*ppFnd)->Which() )
> to
>
> > if ( (*ppFnd)->ISA(SfxVoidItem) )

Yes, there are quite a few evil cornercases lurking there. Before changing
those conditionals, I think it is a good idea to just add assertions that make
sure these assumptions are kept consistent all the time. A start would be to
check that when an item is put in the set, to check:
- If it has a non-zero Which-Id
- and is not a SfxVoidItem
any client code that does something different (even explicitly putting a
SfxVoidItem), is abusing the API and should be fixed by making it call
DisableItem instead). Once you are sure that is cleaned up, one can carefully
go on about changing the conditions. Some care is still needed as in theory,
someone might have e.g. put a non-zero, non-SfxVoidItem and later set the
WhichId on the item to 0.

Of course, we then need to send out a team to hunt down those responsible and
whack them with a large trout in their sleep, if we ever come across such a
case.

So, to change the conditionals, we need to make sure the state is consistent.
Thus:
- adding the asserts
- run e.g. subsequentcheck to see if they every pop something up

After that: Carefully start migrating the conditionals.

Does that make sense?

Best,

Bjoern