Refactoring LibreOffice: VCL FontCharMap

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.