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-redundant-smartptr-get doesn't report get() usage in conditions #26178

Closed
EugeneZelenko opened this issue Dec 11, 2015 · 15 comments
Closed
Labels
bugzilla Issues migrated from bugzilla clang-tidy

Comments

@EugeneZelenko
Copy link
Contributor

Bugzilla Link 25804
Resolution FIXED
Resolved on Sep 26, 2016 17:38
Version unspecified
OS Linux
CC @zmodem,@kirillbobyrev,@Xazax-hun

Extended Description

I tried to run trunk Clang-tidy readability-redundant-smartptr-get for LLDB code (source/Target/Target.cpp) (as of r255174) and this check doesn't report get usage in conditions like:

if (m_process_sp.get())
if (m_source_manager_ap.get() == NULL)
if (rhs.m_thread_spec_ap.get() != NULL)

Such cases should be handled as http://en.cppreference.com/w/cpp/memory/shared_ptr/operator_bool.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 11, 2015

Please provide a self-contained repro (system #includes are fine).

@EugeneZelenko
Copy link
Contributor Author

Simple test case

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 11, 2015

The cases that are missing are:

  • Explicit bool conversion contexts, like 'if (ptr.get())'
  • Comparison with NULL. (comparison with nullptr already work).

@EugeneZelenko
Copy link
Contributor Author

The cases that are missing are:

  • Explicit bool conversion contexts, like 'if (ptr.get())'
  • Comparison with NULL. (comparison with nullptr already work).

I tried Clang-tidy trunk version on attached example and comaprison with nullptr are not reported.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 11, 2015

Are you using libc++?
It might be that the check is not working properly with libc++.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 11, 2015

This seems to be related with how libc++ deals with ABI compatibility.
Instead of defining types in std, it defines them in std::_LIBCPP_NAMESPACE where _LIBCPP_NAMESPACE is an inline namespace.
The clang-tidy checks are specifically matching against "::std::xxx" and that is failing because the names are actually "::std::_LIBCPP_NAMESPACE::xxx".

We could try to fix hasName() to understand inline namespaces, or we could fix the checks to match the name differently.

@EugeneZelenko
Copy link
Contributor Author

Are you using libc++?
It might be that the check is not working properly with libc++.

Yes, I used libc++. Just in case complete compile command line parameters:

-Weverything -std=c++11 -m64 -stdlib=libc++ -lc++abi -lc++ SmartPtrGet.cpp -o SmartPtrGet.exe

@Xazax-hun
Copy link
Collaborator

Fixing hasName to deal with inline namespaces seems like a better idea to me. Littering every STL specific checks with library implementation details might result in hard to maintain code.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 14, 2015

Sent http://reviews.llvm.org/D15506 for review

@EugeneZelenko
Copy link
Contributor Author

Fixed in r259898.

@EugeneZelenko
Copy link
Contributor Author

I run this check through LLDB code and next cases still not detected:

  • Explicit bool conversion contexts, like 'if (ptr.get())'
  • Comparison with NULL. (comparison with nullptr already work).

@zmodem
Copy link
Collaborator

zmodem commented Feb 23, 2016

I'm not treating this as a 3.8 blocker.

@kirillbobyrev
Copy link
Member

D24893 up for review.

@kirillbobyrev
Copy link
Member

Fixed in r282386.

@EugeneZelenko
Copy link
Contributor Author

Thank you for help, Kirill!

@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

5 participants