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 25804 - readability-redundant-smartptr-get doesn't report get() usage in conditions
Summary: readability-redundant-smartptr-get doesn't report get() usage in conditions
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: Samuel Benzaquen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-10 20:16 PST by Eugene Zelenko
Modified: 2016-09-26 17:38 PDT (History)
8 users (show)

See Also:
Fixed By Commit(s):


Attachments
Simple test case (724 bytes, application/octet-stream)
2015-12-11 13:07 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-10 20:16:41 PST
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.
Comment 1 Alexander Kornienko 2015-12-11 08:36:11 PST
Please provide a self-contained repro (system #includes are fine).
Comment 2 Eugene Zelenko 2015-12-11 13:07:07 PST
Created attachment 15433 [details]
Simple test case
Comment 3 Samuel Benzaquen 2015-12-11 14:00:20 PST
The cases that are missing are:
 - Explicit bool conversion contexts, like 'if (ptr.get())'
 - Comparison with NULL. (comparison with nullptr already work).
Comment 4 Eugene Zelenko 2015-12-11 14:06:22 PST
(In reply to comment #3)
> 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.
Comment 5 Samuel Benzaquen 2015-12-11 14:12:11 PST
Are you using libc++?
It might be that the check is not working properly with libc++.
Comment 6 Samuel Benzaquen 2015-12-11 14:28:31 PST
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.
Comment 7 Eugene Zelenko 2015-12-11 14:47:20 PST
(In reply to comment #5)
> 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
Comment 8 Gábor Horváth 2015-12-14 15:08:53 PST
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.
Comment 9 Samuel Benzaquen 2015-12-14 15:23:33 PST
Sent http://reviews.llvm.org/D15506 for review
Comment 10 Eugene Zelenko 2016-02-05 20:03:35 PST
Fixed in r259898.
Comment 11 Eugene Zelenko 2016-02-18 16:00:58 PST
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).
Comment 12 Hans Wennborg 2016-02-23 15:34:13 PST
I'm not treating this as a 3.8 blocker.
Comment 13 Kirill Bobyrev 2016-09-24 17:56:25 PDT
D24893 up for review.
Comment 14 Kirill Bobyrev 2016-09-26 02:50:51 PDT
Fixed in r282386.
Comment 15 Eugene Zelenko 2016-09-26 17:38:55 PDT
Thank you for help, Kirill!