LLVM Bugzilla is read-only and represents the historical archive of all LLVM issues filled before November 26, 2021. Use github to submit LLVM bugs

Bug 25812 - readability-container-size-empty doesn't work as intended with real vector
Summary: readability-container-size-empty doesn't work as intended with real vector
Status: RESOLVED FIXED
Alias: None
Product: clang-tools-extra
Classification: Unclassified
Component: clang-tidy (show other bugs)
Version: unspecified
Hardware: PC Linux
: P normal
Assignee: Gábor Horváth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-11 19:51 PST by Eugene Zelenko
Modified: 2016-02-09 15:08 PST (History)
4 users (show)

See Also:
Fixed By Commit(s):


Attachments
Proprocessed source (75.58 KB, application/gzip)
2016-02-08 12:37 PST, Eugene Zelenko
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eugene Zelenko 2015-12-11 19:51:19 PST
I run trunk Clang-tidy with readability-container-size-empty check on LLDB code.

This check missed vector.size() == 0 in source/Target/ThreadPlanTracer.cpp.

I tried standard test readability-container-size-empty.cpp with real vector by replacing local definition with #include <vector>.

Check didn't report anything when compiled with "-stdlib=libc++ -lc++abi -lc++" and only 4 warnings when compiled with "-stdlib=libstdc++ --gcc-toolchain=<path to GCC 4.8.3>".

Will be good idea to try other containers too.
Comment 1 Gábor Horváth 2015-12-12 05:36:53 PST
Thank you for reporting this. Should be fixed in r255431.
Comment 2 Eugene Zelenko 2015-12-14 12:43:10 PST
Thank you for fixes!

However some problems remains.

1) There is discrepancy in number of warnings between vector stub and GCC 4.8 STL implementation: 26 warnings with 5 suppressed vs. 22 warnings and 1 suppressed.

2) Check doesn't report warnings with LLVM STL implementation is used. See bug 25804 for analysis of similar problem.
Comment 3 Gábor Horváth 2015-12-17 05:43:52 PST
Unfortunately I could not reproduce, but I have a different version of GCC STL installed. 

Can you retest it once http://reviews.llvm.org/D15506 is committed?
Comment 4 Alexander Kornienko 2015-12-17 08:45:47 PST
In case libc++ and libstdc++ differ enough for the check to only work on one of them, we need to add tests with the mocks simulating both libraries.
Comment 5 Gábor Horváth 2015-12-21 03:52:50 PST
I do not fully agree. In case the difference is the result of the usage of inline namespaces, then every STL related checks would have duplicated mocks.
I think once inline namespaces are supported by hasName, and this is the only difference that cause issues, it should be ok to not test this on every STL related checks.
Comment 6 Alexander Kornienko 2015-12-22 17:07:03 PST
(In reply to comment #5)
> I do not fully agree. In case the difference is the result of the usage of
> inline namespaces, then every STL related checks would have duplicated mocks.
> I think once inline namespaces are supported by hasName, and this is the
> only difference that cause issues, it should be ok to not test this on every
> STL related checks.

I agree, if the use of inline namespaces is the only difference, then we don't need separate mocks in each test (though testing this at least in one place seems useful).
Comment 7 Eugene Zelenko 2016-02-05 20:13:04 PST
I tried r259898 but check still didn't work for libc++.
Comment 8 Gábor Horváth 2016-02-06 03:32:25 PST
Could you upload a preprocessed source file to reproduce the error?
Comment 9 Eugene Zelenko 2016-02-08 12:37:10 PST
Created attachment 15857 [details]
Proprocessed source

Just in case, I used next Clang command-line parameters in addition to -o and -c: -std=c++11 -stdlib=libc++.
Comment 10 Eugene Zelenko 2016-02-08 19:26:06 PST
I think problem happens because of next size() attribute:

__attribute__ ((__visibility__("hidden"), __always_inline__))
Comment 11 Gábor Horváth 2016-02-09 04:22:23 PST
Should be fixed by r260217.
Comment 12 Eugene Zelenko 2016-02-09 15:08:31 PST
Thank you for quick fix!