[Interest] Klocwork errors in Qt

[André Pönitz]

On Wed, Dec 04, 2019 at 02:36:37PM +1300, Christian Gagneraud wrote:
> On Wed, 4 Dec 2019 at 13:05, Thiago Macieira <thiago.macieira at intel.com> wrote:
> > > IMHO, it is legit to write code like:
> > > const QUrl url(urlString);
> > > const auto appId = url.host() + "." + url.path().mid(1);
> > > doSomeThing(appId); // void doSomeThing(const QString &stuff);
> > >
> > > Yet it results in crashes, apparently because of QStringBuilder
> > > holding references to temporary implicitly shared Qt object. I would
> > > expect QStringBuilder to somehow grab a reference to this implicitely
> > > shared object (eg. by incrementing a ref counter).
> >
> > We'd lose performance on the common case by doing atomic reference counting up
> > and down. QStringBuilder would need to hold (shallow, implicitly-shared)
> > copies of the QString values returned by QUrl::host() and QString::mid(). One
> > can argue that the cost is reasonable and worth the extra safety it gives.
> 
> My point is that this sort of bugs are really hard to detect during
> code review. This is subtle & nasty 'feature' of QStringBuilder (a
> choice in its implementation).

I beg to differ.

While QStringBuilder plays some role, the main problem here is the
use of 'auto' in circumstances where the type is _not_ irrelevant.

That's a feature of 'auto' already, not introduced by QStringBuilder:
Replacing 'char' by 'auto' in the following changes behavior in plain C++:

    void say_hi(char) { std::cout << "Hi!" << std::endl; }
    void say_hi(int) { std::cout << "BUH!" << std::endl; }

    int main()
    {
        char a = 1;  // Not auto.
        say_hi(a);
    }   
    

> QStringBuilder has no documentation (that i could find),

https://doc.qt.io/qt-5/qstring.html says why:

   In 4.6, an internal template class QStringBuilder has been added along
   with a few helper functions. This class is marked internal and does not
   appear in the documentation, because you aren't meant to instantiate
   it in your code.

[Yes, one could explicitly point to 'auto' here, but that had a slightly
different meaning at the time QStringBuilder was written...]

So to actually experience this behaviour one would have to
 
  1. Opt-in to using a feature without being aware of the consequences
  2. Have too liberal 'auto' usage rule without being aware of the consequences
  3. Use both in an unsupported combination
  4. Do not notice that
  5. Do not use tools recognizing this
  6. Have a reviewer that does not understand the code either but still accepts it.

Of course, all these things are known to happen in practice individually.
But when all six happen at the same time I am tempted to bet that the rest of 
the code is in a state that this auto-AND-QStringBuilder use doesn't change a lot.

> BTW, Clazy has a check for that very specific case, which just show
> how dangerous is this QStringBuilder.

    auto-unexpected-qstringbuilder

    Finds places where auto is deduced to be QStringBuilder instead of QString,
    which introduces crashes.

Clazy warns (correctly) about auto-AND-QStringBuilder. The dangerous part
here is 'this auto' as well.

Also, really Nobody is forced to use QStringBuilder, it does not happen by
accident either. You have to opt in. If you expect problems with it,
do not opt in. 

Andre'