clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

Kristof Beyls via cfe-dev
Hello,

I'm getting an implicit conversion warning on a piece of code that (from
my understanding) I shouldn't. Most likely my understanding is incorrect
:-)

The piece of code is:

bool test1(const int* ptr, int other)
{
  if (ptr) {
    return *ptr == other;
  }
  return false;
}

bool test2(const int* ptr, int other)
{
  return ptr && *ptr == other;
}

With AllowPointerConditions unset (0) I'm getting warnings both from
test1 and test2. With AllowPointerConditions set (1) I'm still getting a
warning from test2. Why is that? I guess that I don't understand the
'conditional' part of this option...

I'm using clang-tidy-8 (LLVM version 8.0.1) with the following
configuration:

-config="{Checks: '*', CheckOptions: [{key: readability-implicit-bool-conversion.AllowPointerConditions, value: 1} ]}"

  - Simon
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

Kristof Beyls via cfe-dev
On 2019-09-12 09:55, Simon Sandström via cfe-dev wrote:

> Hello,
>
> I'm getting an implicit conversion warning on a piece of code that (from
> my understanding) I shouldn't. Most likely my understanding is incorrect
> :-)
>
> The piece of code is:
>
> bool test1(const int* ptr, int other)
> {
>    if (ptr) {
>      return *ptr == other;
>    }
>    return false;
> }
>
> bool test2(const int* ptr, int other)
> {
>    return ptr && *ptr == other;
> }
>
> With AllowPointerConditions unset (0) I'm getting warnings both from
> test1 and test2. With AllowPointerConditions set (1) I'm still getting a
> warning from test2. Why is that? I guess that I don't understand the
> 'conditional' part of this option...

I believe both tests are equivalent as far as C++ language semantics go
- you are using a pointer as a boolean inside a conditional expression.
In the first case you have an explicit "if" yet in the second you there
is a logical expression inside the "return". That is, you can rewrite
both with "ptr != nullptr" to silence the warning.

Oleg.

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

Kristof Beyls via cfe-dev
On 13/09, Oleg Smolsky wrote:

> On 2019-09-12 09:55, Simon Sandström via cfe-dev wrote:
> > Hello,
> >
> > I'm getting an implicit conversion warning on a piece of code that (from
> > my understanding) I shouldn't. Most likely my understanding is incorrect
> > :-)
> >
> > The piece of code is:
> >
> > bool test1(const int* ptr, int other)
> > {
> >    if (ptr) {
> >      return *ptr == other;
> >    }
> >    return false;
> > }
> >
> > bool test2(const int* ptr, int other)
> > {
> >    return ptr && *ptr == other;
> > }
> >
> > With AllowPointerConditions unset (0) I'm getting warnings both from
> > test1 and test2. With AllowPointerConditions set (1) I'm still getting a
> > warning from test2. Why is that? I guess that I don't understand the
> > 'conditional' part of this option...
>
> I believe both tests are equivalent as far as C++ language semantics go -
> you are using a pointer as a boolean inside a conditional expression. In the
> first case you have an explicit "if" yet in the second you there is a
> logical expression inside the "return". That is, you can rewrite both with
> "ptr != nullptr" to silence the warning.
>
> Oleg.
>

Well yes, I can silence it by explicitly comparing it to nullptr, but
shouldn't it by silenced by using the AllowPointerConditions option as
well? In both cases above, that is.

- Simon
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

Kristof Beyls via cfe-dev
In the second case, you're not using it in a condition context -- it's just a normal boolean-valued expression.  The flag only disables the check when these implicit casts appear in the context of "conditions": if, while, for, etc.  Here's the relevant code:


The documentation (https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html) could arguably use some clarification.

On Sat, Sep 14, 2019 at 7:05 AM Simon Sandström via cfe-dev <[hidden email]> wrote:
On 13/09, Oleg Smolsky wrote:
> On 2019-09-12 09:55, Simon Sandström via cfe-dev wrote:
> > Hello,
> >
> > I'm getting an implicit conversion warning on a piece of code that (from
> > my understanding) I shouldn't. Most likely my understanding is incorrect
> > :-)
> >
> > The piece of code is:
> >
> > bool test1(const int* ptr, int other)
> > {
> >    if (ptr) {
> >      return *ptr == other;
> >    }
> >    return false;
> > }
> >
> > bool test2(const int* ptr, int other)
> > {
> >    return ptr && *ptr == other;
> > }
> >
> > With AllowPointerConditions unset (0) I'm getting warnings both from
> > test1 and test2. With AllowPointerConditions set (1) I'm still getting a
> > warning from test2. Why is that? I guess that I don't understand the
> > 'conditional' part of this option...
>
> I believe both tests are equivalent as far as C++ language semantics go -
> you are using a pointer as a boolean inside a conditional expression. In the
> first case you have an explicit "if" yet in the second you there is a
> logical expression inside the "return". That is, you can rewrite both with
> "ptr != nullptr" to silence the warning.
>
> Oleg.
>

Well yes, I can silence it by explicitly comparing it to nullptr, but
shouldn't it by silenced by using the AllowPointerConditions option as
well? In both cases above, that is.

- Simon
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

Kristof Beyls via cfe-dev
On 18/09, Yitzhak Mandelbaum wrote:

> In the second case, you're not using it in a condition context -- it's just
> a normal boolean-valued expression.  The flag only disables the check when
> these implicit casts appear in the context of "conditions": if, while, for,
> etc.  Here's the relevant code:
>
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp#L220
>
> The documentation (
> https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html)
> could arguably use some clarification.
>
> On Sat, Sep 14, 2019 at 7:05 AM Simon Sandström via cfe-dev <
> [hidden email]> wrote:
>

Thank you, that explains it. Besides updating the documentation, do you
think it would be useful to add a new option to allow implicit casts
like in the second case? AllowPointerReturnStatement or whatever.

Maybe there are other issues that can arise from implicitly casting
pointer to bool in return statement that cannot happen in a condition
context? Or it's just preferred to explicitly check against nullptr in a
return statement?

- Simon
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: clang-tidy: problem with readability-implicit-bool-conversion AllowPointerConditions

Kristof Beyls via cfe-dev
If I'm understanding your argument correctly, you're saying that if we allow pointer-to-bool in a conditional context because it is "tagged" by the context, then we can similarly argue that "return" tags the context?  If so, I see your point, but "return" doesn't imply a bool type whereas the others do.  So, it's different than an arbitrary expression context, but, IMO, not enough.

Still, this is all a matter of taste, so if you are willing to write the patch to add that configuration option, I'd think you'll get a fair consideration from reviewers.

On Fri, Sep 20, 2019 at 6:52 AM Simon Sandström <[hidden email]> wrote:
On 18/09, Yitzhak Mandelbaum wrote:
> In the second case, you're not using it in a condition context -- it's just
> a normal boolean-valued expression.  The flag only disables the check when
> these implicit casts appear in the context of "conditions": if, while, for,
> etc.  Here's the relevant code:
>
> https://github.com/llvm/llvm-project/blob/master/clang-tools-extra/clang-tidy/readability/ImplicitBoolConversionCheck.cpp#L220
>
> The documentation (
> https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html)
> could arguably use some clarification.
>
> On Sat, Sep 14, 2019 at 7:05 AM Simon Sandström via cfe-dev <
> [hidden email]> wrote:
>

Thank you, that explains it. Besides updating the documentation, do you
think it would be useful to add a new option to allow implicit casts
like in the second case? AllowPointerReturnStatement or whatever.

Maybe there are other issues that can arise from implicitly casting
pointer to bool in return statement that cannot happen in a condition
context? Or it's just preferred to explicitly check against nullptr in a
return statement?

- Simon

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

smime.p7s (6K) Download Attachment