Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

readability-container-size-empty doesn't work as intended with real vector #26186

Closed
EugeneZelenko opened this issue Dec 12, 2015 · 13 comments
Closed
Assignees
Labels
bugzilla Issues migrated from bugzilla clang-tidy

Comments

@EugeneZelenko
Copy link
Contributor

Bugzilla Link 25812
Resolution FIXED
Resolved on Feb 09, 2016 15:08
Version unspecified
OS Linux
CC @Xazax-hun

Extended Description

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 .

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.

@EugeneZelenko
Copy link
Contributor Author

assigned to @Xazax-hun

@Xazax-hun
Copy link
Collaborator

Thank you for reporting this. Should be fixed in r255431.

@EugeneZelenko
Copy link
Contributor Author

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.

@Xazax-hun
Copy link
Collaborator

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?

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 17, 2015

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.

@Xazax-hun
Copy link
Collaborator

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.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 23, 2015

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).

@EugeneZelenko
Copy link
Contributor Author

I tried r259898 but check still didn't work for libc++.

@Xazax-hun
Copy link
Collaborator

Could you upload a preprocessed source file to reproduce the error?

@EugeneZelenko
Copy link
Contributor Author

Proprocessed source
Just in case, I used next Clang command-line parameters in addition to -o and -c: -std=c++11 -stdlib=libc++.

@EugeneZelenko
Copy link
Contributor Author

I think problem happens because of next size() attribute:

attribute ((visibility("hidden"), always_inline))

@Xazax-hun
Copy link
Collaborator

Should be fixed by r260217.

@EugeneZelenko
Copy link
Contributor Author

Thank you for quick fix!

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang-tidy
Projects
None yet
Development

No branches or pull requests

3 participants