This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add check bugprone-unique-ptr-array-mismatch.
ClosedPublic

Authored by balazske on May 25 2023, 5:17 AM.

Diff Detail

Event Timeline

balazske created this revision.May 25 2023, 5:17 AM
balazske requested review of this revision.May 25 2023, 5:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 5:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL accepted this revision.EditedMay 25 2023, 9:43 AM

LGTM, But:

  • Align check description before committing.
  • Consider adding some test with std::unique_ptr behind typedef.
  • Consider adding test with unique_ptr depend on template argument but without specializations (like f_tmpl but without f5 function).

General issue that I got (no need to change that) with this test (and test for shared_ptr) is that actually there is no issue here.
For POD types it actually doesn't mater if you use delete or delete[]. This of course isn't portable and fall into implementation specific.
But can read about this in https://itanium-cxx-abi.github.io/cxx-abi/abi.html#array-cookies
This is why I would like to see NonPOD types instead of POD ones to be used with unique_ptr.
On GCC/Clang memory will be fully released (POD) types or corrupted (non POD types).

clang-tools-extra/clang-tidy/bugprone/UniquePtrArrayMismatchCheck.h
17–24

this should be same as in release notes ("Finds initializations of C++ unique pointers to non-array type that are initialized with an array.")

This revision is now accepted and ready to land.May 25 2023, 9:43 AM
balazske updated this revision to Diff 526063.May 26 2023, 7:53 AM
balazske marked an inline comment as done.

Fixed the documentation issue.
Added tests.

The case of fully dependent type (unique_ptr<T>) is not working in this checker and likely not in SharedPtrArrayMismatchCheck. I can fix this in a next patch (for both checkers).
Maybe we can remove the warning in all cases when the type is a POD type (or add a check option)?

PiotrZSL accepted this revision.May 26 2023, 9:02 AM

Maybe we can remove the warning in all cases when the type is a POD type (or add a check option)?

Leave it... From a user perspective, it doesn't mater if type is POD or not.
I was mentioning non POD just because this is one of "undefined behavior" that sometimes is well defined.
But you don't know if project isn't using some custom new[]/delete[]

clang-tools-extra/docs/clang-tidy/checks/bugprone/unique-ptr-array-mismatch.rst
25

remove check name from comment to make it shorter

balazske updated this revision to Diff 526927.May 30 2023, 11:49 PM

removed check name in documentation code comment

balazske marked an inline comment as done.May 30 2023, 11:56 PM