Page MenuHomePhabricator

[clang-tidy] New check misc-uniqueptr-release-unused-retval
AbandonedPublic

Authored by khuttun on Dec 10 2017, 3:28 AM.

Details

Summary

Detects calls to std::unique_ptr::release where the return value is unused.

Diff Detail

Event Timeline

khuttun created this revision.Dec 10 2017, 3:28 AM

May be bugprone is better module then misc?

docs/ReleaseNotes.rst
60

Please put in list of new checks in alphabetical order.

63

Please copy first statement from documentation.

Eugene.Zelenko set the repository for this revision to rL LLVM.
Eugene.Zelenko added a project: Restricted Project.

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

As not reviewer I agree for it to be in bugprone.

alexfh edited edge metadata.Dec 11 2017, 8:09 AM

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

aaron.ballman edited edge metadata.Dec 11 2017, 8:48 AM

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

There's a push in WG21 to mark more of the library with [[nodiscard]]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0600r1.pdf

If we have a check for this, I do not think it should be specific to unique_ptr::release(), but instead be more broadly applicable to APIs that should be marked [[nodiscard]] but are not (currently). P0600R1 is a good place to start, but I'm guessing there are POSIX APIs (among others) that would also qualify.

May be bugprone is better module then misc?

Maybe. I can move it if all the reviewers think that it would be better suited there.

Yup, bugprone- should be a better category for this, IMO.

I wonder whether libc++ folks are interested in marking unique_ptr::release() with __attribute__ ((warn_unused_result)). A compiler warning (with -Werror) would be a better protection against this kind of a bug.

There's a push in WG21 to mark more of the library with [[nodiscard]]: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0600r1.pdf

If we have a check for this, I do not think it should be specific to unique_ptr::release(), but instead be more broadly applicable to APIs that should be marked [[nodiscard]] but are not (currently). P0600R1 is a good place to start, but I'm guessing there are POSIX APIs (among others) that would also qualify.

P0600R1 seems to suggest quite conservative approach (and rightly so, IMO) in taking [[nodiscard]] in use in std lib. For example it proposes *not* to use it with unique_ptr::release. I think there could still be room for static unused ret val checking for parts of std lib not covered by P0600R1, but also for boost, POSIX APIs etc.

What do you think about making this check fully configurable through the check options? The options could list the functions to check. The documentation could suggest some candidate functions from std lib to configure checks for.

What do you think about making this check fully configurable through the check options? The options could list the functions to check. The documentation could suggest some candidate functions from std lib to configure checks for.

That sound reasonable. A similar approach has been used for cppcoreguidelines-no-malloc already.

P0600R1 seems to suggest quite conservative approach (and rightly so, IMO) in taking [[nodiscard]] in use in std lib. For example it proposes *not* to use it with unique_ptr::release. I think there could still be room for static unused ret val checking for parts of std lib not covered by P0600R1, but also for boost, POSIX APIs etc.

What do you think about making this check fully configurable through the check options? The options could list the functions to check. The documentation could suggest some candidate functions from std lib to configure checks for.

Agreed; that's largely what I was suggesting with my previous comment. I think this should be a generic, configurable check that is prepopulated with some APIs that make sense. You can use P0600R1 as a starting point, but I'd encourage you to find a pretty decent (but still conservative) list of APIs.

khuttun abandoned this revision.Jan 1 2018, 4:45 AM

Closing this as more general check is being reviewed here: https://reviews.llvm.org/D41655