`clang-tidy` check request: a `new` expression that is implicitly converted to `bool`

[chandlerc]

Consider the following code:

struct S {};

void f(bool flag);

void g() {
  // Check should fire here:
  f(new S());
}

This new expression seems clearly a bug. This feels like a case clang-tidy could error on easily, similar to other bugprone checks. It might even make sense as a warning. The only use case I can imagine where this isn't a bug due to leaking memory is for types that register themselves in some collection when heap allocated, so the pointer produced by new isn't actually needed and can be used as a bool. But it is always true, and so it seems much more clear to write:

void g() {
  (void)new S();
  f(true);
}

And not get a warning. This seems to more clearly show that the result of the new expression is intentionally discarded.

[llvmbot]

@llvm/issue-subscribers-clang-tidy

[PiotrZSL]

This is detected by:

:7:5: warning: initializing non-owner argument of type 'bool' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory]
    7 |   f(new S());
      |     ^~~~~~~
:7:5: warning: implicit conversion 'S *' -> bool [readability-implicit-bool-conversion]
    7 |   f(new S());
      |     ^
      |             != nullptr
:8:1: warning: Potential memory leak [clang-analyzer-cplusplus.NewDeleteLeaks]
    8 | }

Do you still want separate check?

[chandlerc]

The first two I think aren't great options. Those are great checks, but they check a much broader set of things, and as a consequence can't always be enabled in projects. For example, the second one will warn on any implicit conversion, not just one from a new expression.

The third one would be plausible, but it seems both much more expensive and maybe harder to enable if there are any false-positives. I might still try enabling it since that does seem like a good way to get here.

But it seems like if anyone else is hits this, it might be worthwhile to have a much simpler and somewhat more boring check. There are a bunch of bugprone checks or warnings that technically are caught with the static analyzer, but I think many of the checks and warnings are still worthwhile because they're simpler and more easily enabled. Whether this check is worthwhile, no idea. I just saw it and was surprised it compiled so figured I'd file a bug just in case. =]

[PiotrZSL]

Ok, then looks like we may need to implement bugprone-new-bool-conversion, and looks like even non-implicit conversion would be a bug here.

[PiotrZSL]

https://reviews.llvm.org/D157188