Bug 367484 - reserve-candidate warnings for QVarLengthArray
Summary: reserve-candidate warnings for QVarLengthArray
Status: RESOLVED FIXED
Alias: None
Product: clazy
Classification: Developer tools
Component: general (show other bugs)
Version: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Unassigned bugs mailing-list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-18 09:27 UTC by Mathias Hasselmann
Modified: 2016-08-21 19:06 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Hasselmann 2016-08-18 09:27:06 UTC
When filling a QVarLengthArray clazy wrongly emits reserve-candidate warnings:

    constexpr int preallocSize = 160;
    QVarLengthArray<RawDataEntry, preallocSize> initialEntries;

    for (int i = EntryId::FooFirst; i <= EntryId::FooLast; ++i)
        initialEntries.append(RawDataEntry{i, false}); // <- reserve-candidates warning
    for (int i = EntryId::BarFirst; i <= EntryId::BarLast; ++i)
        initialEntries.append(RawDataEntry{i, false}); // <- reserve-candidates warning
Comment 1 Sergio Martins 2016-08-19 13:46:08 UTC
Wouldn't a reserve make sense in that example ?
How do you know FooLast - FooFirst isn't bigger than 160 ?
Comment 2 Mathias Hasselmann 2016-08-19 14:14:09 UTC
(In reply to Sergio Martins from comment #1)
> Wouldn't a reserve make sense in that example ?
Not at all. The purpose of QVarLengthArray is to save the reserve, or rather to do it right on the stack - therefore the pre-alloc parameter.

> How do you know FooLast - FooFirst isn't bigger than 160 ?
Because this are program specific constants. As they are constexpr clazy can evaluate them them. I'd expect clang to have API for that evaluations as compilers have to do those constexpr evaluations all the time.
Comment 3 Sergio Martins 2016-08-19 14:44:14 UTC
(In reply to Mathias Hasselmann from comment #2)
> (In reply to Sergio Martins from comment #1)
> > Wouldn't a reserve make sense in that example ?
> Not at all. The purpose of QVarLengthArray is to save the reserve, or rather
> to do it right on the stack - therefore the pre-alloc parameter.

I usually use QVarLengthArray::reserve() anyway, since QVLA will use the heap when bigger than preallocSize. reserve(X) is a NO-OP for the cases where X <= preallocSize, so there's no harm.

But I understand the warning is annoying for you, since it's all using compile time constants so there's no chance of spilling onto the heap.
Comment 4 Mathias Hasselmann 2016-08-19 18:18:10 UTC
I also understand your approach to just use reserve(), and it makes sense, still clazy should avoid overly defensive warnings: Clazy will be much more useful for team hygiene when it receives the _perception_ to always be right, to always have a point with its warnings. Honestly I'd love it reaching a state, where it is safe to include Clazy warnings into the -Werror list.
Comment 5 Sergio Martins 2016-08-21 19:06:35 UTC
Git commit e879f0ec34147484c179f320dfd0e77b367435de by Sergio Martins.
Committed on 21/08/2016 at 19:04.
Pushed by smartins into branch 'master'.

reserve-candidates: Don't warn for QVarLengthArray

Unlike the other containers QVLA doesn't need reserve in the majority
of cases since it uses the stack, resulting in false-positives.

There's still the edge case of it using the heap, but that would
take a considerable amount of effort to catch, and besides, users
using QVLA usually know what they are doing.

M  +1    -1    QtUtils.cpp
M  +11   -3    tests/reserve-candidates/main.cpp

http://commits.kde.org/clazy/e879f0ec34147484c179f320dfd0e77b367435de