Revert "utils/IXmlDeserializable: drop useless interface"

[a1rwulf]

This reverts commit f821789.

@MaxKellermann this breaks the "Component Specific Loggin"-Toggle in Settings for me.
I have just bisected it and this revert fixes it, however I did not look how this commit could break it.

How Has This Been Tested?

Tested on Linux X11.

Types of change

• Bug fix (non-breaking change which fixes an issue)
• Improvement (non-breaking change which improves existing functionality)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

[ksooo]

class CSettingDependencyCondition : public CSettingConditionItem

both classes (did) override Deserialize.

Maybe this causes the problem? Did no deep analysis, though.

[MaxKellermann]

Sorry for the breakage, I'll investigate.

[MaxKellermann]

@a1rwulf this revert is correct, and my patch was wrong. But its description was correct:

There is no IXmlDeserializable::Deserialize() call on a possibly polymorphed object

That is true. The thing is that while nobody uses the IXmlDeserializable interface, CBooleanLogicOperation calls CBooleanLogicValue::Deserialize(), and CBooleanLogic calls CBooleanLogicOperation::Deserialize() - both are virtual only because the vtable was inherited from IXmlDeserializable. Even if nobody uses the interface, the Deserialize() method needs to be virtual.
I find this coding style confusing and I wouldn't design it that way; if I define an interface, I expect callers to use that interface, and not classes derived from it. Anyway, my confusion was my mistake, and this needs a revert.

Thanks for the bisect and the PR. I suggest merging it.

[ksooo]

I suggest merging it.

Thanks, Max. Done.

[yol]

Shouldn't we at least add a comment to IXmlDeserializable to prevent this from happening again?

[ksooo]

Shouldn't we at least add a comment to IXmlDeserializable to prevent this from happening again?

Fixing the strange design Max mentioned would even be better.

[a1rwulf]

That is true. The thing is that while nobody uses the IXmlDeserializable interface, CBooleanLogicOperation calls CBooleanLogicValue::Deserialize(), and CBooleanLogic calls CBooleanLogicOperation::Deserialize() - both are virtual only because the vtable was inherited from IXmlDeserializable. Even if nobody uses the interface, the Deserialize() method needs to be virtual.
I find this coding style confusing and I wouldn't design it that way; if I define an interface, I expect callers to use that interface, and not classes derived from it. Anyway, my confusion was my mistake, and this needs a revert.

We have quite some of such places in our code and your effort to do cleanups is much appreciated.

[ksooo]

and your effort to do cleanups is much appreciated.

definitely +1

[MaxKellermann]

I'd still remove IXmlDeserializable completely in the long run, and instead add virtual Deserialize() methods to those base classes that need it. Having one central interface for it that is used by nobody directly only adds confusion and overhead.

[ksooo]

I agree. If you you are in the mood to do this, you are more than welcome. :-)