You can(not) have a void namespace in C++...

Update: So this was discussed on IRC, and it turns out I've got this all entirely wrong. It's actually much simpler - it turns out you don't need a space before the scope operator thus you can legally have:

void::TheClass::TheFunction(int n1, int n2) {}

This is the same as:

void ::TheClass::TheFunction(int n1, int n2) {}

and:

void     ::TheClass      ::TheFunction(int n1, int n2) {}

--------

I ran doxygen over the vcl module in LibreOffice and I got a lot of warnings and errors. I am now making a concerted effort to fix these warnings because I actually find doxygen generates good info on the classes, and I rather like the collaboration and inheritance diagrams it produces.

However, a strange error that it produced was:

/home/chris/repos/libreoffice/vcl/source/control/combobox.cxx:1322 warning: no uniquely matching class member found for void::ComboBox::Impl::ImplUserDrawHandler(UserDrawEvent *pEvent)

"A void namespace in the LibreOffice codebase?" I thought. How could this be? How is this even possible?

Sure enough, the definition was void::ComboBox::Impl:ImplUserDrawHandler(UserDrawEvent*)!

In an effort to work out if this was what was intended (which I was very doubtful of) I tracked down the commit, which converted the old style IMPL_LINK macro to a boost signal. It was definitely not intended!

I then tracked down where the C++ standard is held and reviewed the C++14 draft (I can't afford to spend US$216 on the official ISO standard). Section 7.3 deals with namespaces:

Amazingly (to my mind) there is no restriction on the namespace identifier, thus it is entirely possible to accidentally create a void, int, float or any other reserved C++ keyword as a namespace!

I wonder if it might be worthwhile for compilers to spit out a warning if they see this? Perhaps we need to write a clang plugin to detect this sort of thing in LibreOffice. The only reason this hasn't had an impact is that it doesn't appear that anything is currently using user-defined draw signals on comboboxes.

I pushed commit 86338fbb94324ed to fix this.

Possible 15 year old regression

A few days ago, I filed bug 96826 ("Typewriter attribute not given enough weight when finding font based on attributes") against LibreOffice. Basically, I've been refactoring VCL font code and I was very interested to see what my predecessors had been doing before me. This meant that I actually read pretty much all of the commits I could find all the way back to the year 2000 for that file, which held much of the font code for VCL. 

One of the functions was originally in a class called ImplFontCache, which had a get() function that worked as a font mapper. It basically does a running total of a number of weighted values depending on the "strength" or "quality" of the font matching attribute. One of those attributes was to check if the font is a fixed-width font, then check to see if the font is a typewriter style font, giving a deliberately higher weighting to typewriter style fonts. 

Anyway, on the 29th June 2001 someone with the initials th (no idea who this was, nor is it really important) did some tweaks to the function in an attempt to improve font mapping in commit 925806de6. To ensure that the typewriter style was given a weighted value, they have multiplied the value by 2. However, they appear to have accidentally removed a zero from the weighting, thus reducing the weighting of the typewriter style fonts, not increased it!

As I was reading the code, I thought "whoops, that was unfortunate" and assumed I'd see the error picked up somewhere down the track and fixed. Now either the weighting has little effect on these fonts, or not many people have problems with typewriter fonts not being cleanly mapped by the underlying platform, but this has never been fixed. 

The code has since migrated its way into PhysicalFontCollection::FindFontFamilyByAttributes(), and it's still there!

        // test MONOSPACE+TYPEWRITER attributes
        if( nSearchType & ImplFontAttrs::Fixed )
        {
            if( nMatchType & ImplFontAttrs::Fixed )
                nTestMatch += 1000000*2;
            // a typewriter attribute is even better
            if( ImplFontAttrs::None == ((nSearchType ^ nMatchType) & ImplFontAttrs::Typewriter) )
                nTestMatch += 10000*2;
        }

As with any codebase with a 30 year pedigree, I've not committed a fix because I just don't know if this is something I want to touch. Hence I've logged the bug to see if anyone can shed any light on it. I'll probably follow up on the mailing list in a few weeks time to ping any more experienced developers, but until then this potential bug, which is in the middle of going through puberty, will remain. 

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.

How to build only vcl modules from scratch in LibreOffice

After reviewing the component diagram on the LibreOffice wiki, the following will make the VCL module in LibreOffice:

make sal salhelper store cppuhelper cppu xmlreader registry unoidl dtrans \
     binaryurp dtrans animations jvmfwk jvmaccess javaunohelper stoc i18nlangtag \
     ucbhelper comphelper basegfx tools unotools i18nutil i18npool sot svl vcl

Update: this doesn't always work. Turns out that there is some sort of circular dependency between i18npool and another module, which make sorts out itself. 

I'm now trying:

make CppunitTest_i18npool_test_breakiterator ucb configmgr vcl