I have been looking at the
VCL in LibreOffice, which is its cross-platform widget and window library. Whilst reading the
SalGraphics class (more on this in a future post) I noticed a class called
ImplFontCharMap . Curious, I looked more into it. Why the "Impl"-prefix? What about
FontCharMap?
As it turns out,
ImplFontCharMap is a
Pimpl for
FontCharMap. Now normally a Pimpl has very little code and is not directly accessible by any class other than the class that uses it. A Pimpl allows for better builds in C++, and a number of other reasons. In this case
ImplFontCharMap was doing a LOT.
A font character map in VCL allows a font to be mapped to Unicode codepoints. The VCL font charmap allows you to find a character in the font based on the Unicode codepoint, find the next and previous character supported by the font (these are not necessarily contiguous) and the first and last characters supported. There is also a default charmap, which maps the
BMP plane, including surrogates.
ImplFontCharMap had an internal reference counting mechanism to optimise sharing of charmaps. However, this was better changed to boost's
intrusive_ptr, because frankly that implementation is far more well engineered and tested, not to mention I'm not a fan of maintaining code that isn't really specifically addressing VCL needs. (incidentally, the best rundown I found of intrusive_ptr can be found at
this man's blog) The commit can be found
here. You can see that I've been able to immediately remove
AddReference() and
DeReference().
You will notice, however, that there are a few classes who rely on
ImplFontCharMap (now
ImplFontCharMapPtr, a typedef to an intrusive_ptr) directly. In particular,
SalGraphics was relying on returning the Pimpl! Frankly, that's madness in my view. As I've said, Pimpls are really intended to be tightly coupled to a particular class and should
never be used directly. The class that needs the Pimpl should be used! You can see other side effects, because the Pimpl is really duplicating code that should be in
FontCharMap. This is clearly a bit if a code maintenance nightmare.
Given that a Pimpl is one of the very few concepts in C++ that relies on tightly coupling two classes, I made
FontCharMap a friend class of
ImplFontCharMap and moved most public functions from
ImplFontCharMap to
FontCharMap. I kept the destructor and the functions
getDefaultCharMap() and
isCharMap() but you'll notice I made them private, hence the lowercase first letter of the function names. I do NOT want VCL based code to access the Pimpl! I thought this was a necessary compromise because the logic really was more entwined with the data itself. There is a function
FontCharMap::GetDefaultCharMap() although it's not normally necessary as the default charmap is shared via intrusive_ptr and the default
FontCharMap constructor just returns a reference to the default charmap. I have provided it because you can get a default font charmap that excludes symbols.
I realised at this point, after a chat with Kendy on IRC, that I had dealt with managing the Pimpl for
FontCharMap, but now I was returning raw
FontCharMap pointers. This was defeating my refactor, so I made the typedef
FontCharMapPtr, which is an intrusive_ptr to a
FontCharMap instance. I then refactored the code to use this instead of the raw pointer.
The second commit that implemented this can be found
here.
Finally, I have to have a big shout-out to Caolán McNamara from RedHat who found a rather embarassing bug I introduced and
fixed it. The issue was that I didn't initialize the ref counter variables, except where I did where I set it to 1... which was rather silly of me, though I was reliably informed by Michael Meeks that he has done the same thing in the past.
Anyway, this is how I refactored a relatively small class. It actually took a lot of effort to do. In the end, I created a
unit test to ensure that
FontCharMap was still working correctly.
