Bug 377617 - clazy-qstring-left makes false assumption
Summary: clazy-qstring-left makes false assumption
Status: RESOLVED FIXED
Alias: None
Product: clazy
Classification: Developer tools
Component: general (other bugs)
Version First Reported In: unspecified
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Sergio Martins
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-14 15:16 UTC by Markus Enzenberger
Modified: 2017-03-19 12:27 UTC (History)
2 users (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 Markus Enzenberger 2017-03-14 15:16:47 UTC
The clazy-qstring-left warning makes the false assumption that QString::left(1) can be replaced by the more efficient QString::at(0). This is not true in general.

It is illegal to call QString::at() with an invalid position, but QString::left() may be called with an empty string (in which case it also returns an empty string).

There could exist code, which relies on this and uses QString::left(1) to avoid handling the extra case in the calling code.
Comment 1 Sergio Martins 2017-03-18 11:43:23 UTC
Git commit 4234fabc16cbdf3d14ab05fc20f251fcb04719b2 by Sergio Martins.
Committed on 18/03/2017 at 11:43.
Pushed by smartins into branch '1.1'.

qstring-left: Improve readme

at(0) asserts if string is empty, while left(1) does not

M  +7    -2    src/checks/level1/README-qstring-left.md

https://commits.kde.org/clazy/4234fabc16cbdf3d14ab05fc20f251fcb04719b2
Comment 2 Sergio Martins 2017-03-18 11:47:50 UTC
While that's true, it's still faster to call !isEmpty()+at(0) since they don't allocate memory.

I don't see any solution besides improving the readme
Comment 3 Markus Enzenberger 2017-03-18 17:28:34 UTC
The danger that I see right now is that someone runs clazy on code they didn't write, simply follows the warning to replace left(1) by at(0) and thereby introduces a new, maybe subtle and rarely triggered bug.

The optimal solution would be that clazy recognizes if a check for size 0 already exists in the code.

But as a short-time fix, maybe the warning message could be improved to mention that left(1) can be replaced by at(0) if the string is not empty?
Comment 4 Sergio Martins 2017-03-19 12:27:23 UTC
Git commit dcc6a8500bd079fe2a0d62a19fe177bc892ac897 by Sergio Martins.
Committed on 19/03/2017 at 12:26.
Pushed by smartins into branch '1.1'.

qstring-left: Warn the user to make sure string isn't empty

left(1) doesn't assert if string is empty, but at(0) does, so
inform the user about that in the warning message

M  +1    -1    src/checks/level1/qstring-left.cpp
M  +1    -1    tests/qstring-left/main.cpp.expected

https://commits.kde.org/clazy/dcc6a8500bd079fe2a0d62a19fe177bc892ac897