Code review comment for lp:~vanvugt/compiz-core/fix-940139

On Mon, 27 Feb 2012, Daniel van Vugt wrote:

> Yeah, so much code assumes Region is a pointer already that we could easily delete all the #else clauses and not test for REGION_IS_A_POINTER at all. That would also remove the last remaining references to PrivateRegion so privateregion.h would then need to be removed etc. I only left REGION_IS_A_POINTER in there to show explicitly what changed, and to show anyone trying to port the code to a non-X environment later what they had to do.
>

+1 , although X regions are all client side anyways, so thats not even a
problem.

> I did add a constructor for CompRegion using Region (XRegion) already on line 21. The reason why it has to be protected and CompRegionRef a separate class is because "priv" no longer has room for a flag to indicate whether the CompRegion owns priv. So instead of squeezing a bool into CompRegion and breaking the ABI, I figured the new class CompRegionRef was an adequately clean solution.

Ack.

I understand that you wanted to remove the pimpl and make CompRegion a
thin wrapper, although I wonder what the performance implication of having
the indirection through the pimpl there was. Surely if the pimpl was still
there a cleaner implementation would be to store the region pointer and a
refcount, no ?

I guess the reason for that though, now that I think of it is the
difficulty in expressing in C++ syntax whether or not the wrapper actually
"owns" the underlying resource or not.

> --
> https://code.launchpad.net/~vanvugt/compiz-core/fix-940139/+merge/94715
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz-core/fix-940139 into lp:compiz-core.
>