[Interest] Klocwork errors in Qt

[Giuseppe D'Angelo]

Hi,

Il 04/12/19 13:56, Christian Gagneraud ha scritto:
> On Wed, 4 Dec 2019 at 19:28, Giuseppe D'Angelo via Interest
> <interest at qt-project.org> wrote:
>>
>> Il 04/12/19 02:36, Christian Gagneraud ha scritto:
>>> BTW, Clazy has a check for that very specific case, which just show
>>> how dangerous is this QStringBuilder.
>>> https://github.com/KDE/clazy/blob/master/docs/checks/README-auto-unexpected-qstringbuilder.md
>>
>> As a professional developer, one should strive for keeping a codebase
>> under clazy, clang-tidy, and friends; and always be under -Werror.
> 
> Really? (sarcasm)
> There's what you wish, and there's what you're asked (or allowed) to
> do. Another set of orthogonal concepts...
> We don't seem to live in the same world.

Let me elaborate: the original claim was that QStringBuilder is 
dangerous. It's not (*). The danger can only happen as a misuse of it 
and it's not inherent to QStringBuilder itself, just like other 
countless things in C++ (e.g. returning a reference to a local variable).

Yes, this particular danger is easy to trigger (store the result of a 
concatenation under QSB in a variable declared `auto`, more than 
reasonably expecting a QString result). On the other hand, we have 
tooling that can statically detect the mistake (just like compilers 
today will flag the returning a reference to a local variable).

Now, I do get the argument that not everyone has the chance of using 
such tooling, for valid reasons (e.g. big codebase not portable under 
Clang, etc.). And there are surely costs associated at doing the work of 
making the code compile under different compilers, setting up a CI, 
integrating it with a code review tool, and so on.

My counter-argument is: there are also costs associated at *not* doing 
the above. *Not* using static analysis tools costs something, just like 
not using sanitizers costs something (valgrind isn't exactly fast), just 
like not using C++17 (or 14, or 11!) costs something.

In the specific case, the costs are, for instance:
* the cognitive load of remembering not to use `auto` together with 
QStringBuilder (not to mention, the cognitive load of understanding 
what's the problem in the first place);
* the fact that one has specifically to look for it in a code review;
* the time spent debugging and fixing random crashes happening in some 
codepath due to triggering this bad QStringBuilder behavior;
* the spread of "fake news" about Qt / QString / C++ / auto / whatnot, 
e.g. blaming them for hard-to-diagnose and easy-to-introduce bugs;
** ... and of course the time spent debunking some of these "fake news";
* the fact that the code runs slower than necessary because it's not 
being compiled using QStringBuilder unconditionally.

All of this can be avoided, today, by using a tool that immediately 
picks on the usage of `auto` and tells you "please use explicitly 
QString here".

Now, I can't put a figure on the costs above, of course. I'd say that, 
in the long run, the costs of *not* using as much tooling available as 
possible, the latest standards, etc. will be more than doing the 
necessary work to enable their usage and use them (and keep them enabled).

In other words, there is a trade-off somewhere, with a tipping point.

Typically, C++ applications are not made to last 6-12 months and then be 
thrown out and rewritten. C++ projects are, typically, long-running. 
Hence, I am not so sure of how many projects are actually still on the 
"don't use tooling" side of the tipping point.


C++ is not an easy language. It comes with an *immense* cognitive 
burden. We cannot know everything, and be 100% focused, for every single 
line of code we write or review. We need _great_ tooling to detect as 
much as possible before the code ends up committed, if not in 
production. Only the tooling can save us from the complexity of C++.


(*) I would also add: QStringBuilder should actually be enabled by all 
projects, by default, just like it's enabled inside Qt itself 
(libraries, Qt Creator, etc.). There's no reason for paying for N memory 
allocations instead of 1, when possible, and any codebase I've ever seen 
will try and concatenate more than 2 strings together somewhere. I wish 
there was a super-define in Qt that would enable all the "goodies" 
currently under individual defines: QT_USE_STRINGBUILDER, 
QT_NO_CAST_FROM_ASCII, QT_STRICT_ITERATORS, and the like.

>> There are no excuses for not fixing mistakes that tooling can detect.
> 
> Stop being patronising please. You're not helping.

I was not patronizing. Please see the reasoning above.

The authors of C++ tooling (compilers, static analyzers, linters, 
sanitizers, fuzzers, ...) do a tremendous effort to help us build 
(harder,) better, faster, stronger applications by detecting as many 
potential mistakes as possible; possibly, at compile time.
Every time a compiler flags a warning in our code, please remember that 
someone somewhere had to add that warning to some tool, write the code, 
write the autotests, had it reviewed, merged, and so on. The likely 
reasoning for adding that specific warning was something like "I think 
this pattern in the code is dangerous in some cases, better tell the 
user to double check what they're doing". It's just fair to say that, at 
least, the authors of the warning deserve to be heard (= we enable the 
warning and check if it's indeed pointing at an issue). We can still 
choose to ignore them.



> 
>> Kudos to Sérgio,
> 
> I agree, and kudos to contributors too.
> 
> PS: Yes, static analysers and code instrumentation are good, and we
> should all embrace them, but your email didn't help the community in
> any way. Blaming people without context knowledge is actually counter
> productive.

I stuck with the facts reported above. QStringBuilder is not dangerous 
and should be used as much as possible. The real problem is that 
knowledge about all the available C++ tooling is not as widespread as it 
should be.

Anecdotal: at most Qt conferences, when I give a talk, I ask the 
audience to raise their hands if they know GammaRay, and typically more 
than half of the room doesn't (this happened last time one month ago at 
the Qt World Summit).

Sorry for not clarifying my position in detail before.

My 2 c,
-- 
Giuseppe D'Angelo | giuseppe.dangelo at kdab.com | Senior Software Engineer
KDAB (France) S.A.S., a KDAB Group company
Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com
KDAB - The Qt, C++ and OpenGL Experts

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4329 bytes
Desc: Firma crittografica S/MIME
URL: <http://lists.qt-project.org/pipermail/interest/attachments/20191204/82fbe85d/attachment.bin>