Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I think this check belongs to bugprone module. How about std::unique_ptr?
clang-tools-extra/clang-tidy/misc/SharedPtrArrayMismatchCheck.cpp | ||
---|---|---|
1 ↗ | (On Diff #399970) | Please make it same length and single line as closing comment. |
clang-tools-extra/docs/clang-tidy/checks/misc-shared-ptr-array-mismatch.rst | ||
6 ↗ | (On Diff #399970) | Please make first statement same as in Release Notes. |
clang-tools-extra/clang-tidy/misc/SharedPtrArrayMismatchCheck.cpp | ||
---|---|---|
21–26 ↗ | (On Diff #399970) | If you're not doing anything different in the c'tor I think you can using ClangTidyCheck::ClangTidyCheck; |
39 ↗ | (On Diff #399970) | What about the other constructor overloads? |
104 ↗ | (On Diff #399970) | I'm not sure what this is doing? |
106 ↗ | (On Diff #399970) | Is this to guard against multiple variables being declared at once? |
Wouldn't it be orthogonal?
This check looks for existing make_shared usages, whereas
modernize-make-shared adds new make_shared usages from
new shared_ptr usages. I wouldn't expect modernize-make-shared
to generate mismatched scalar/array shared_ptr instances.
I am not sure if it is correct to have replacement (fixit) for a part of the cases only. It is not possible to make a fixit for every use. In a case like shared_ptr<int> p1{new int}, p2{new int[10]}; the type can not be changed without other changes. For this reason the replacement is offered only for single-variable declarations.
Is there a fast way to find single-variable field declaration?
class A { shared_ptr<int> F1{new int}, F2{new int[10]}; };
The FieldDecl does not contain information like DeclStmt::isSingleDecl. The only way looks like to check the source locations.
This check looks for constructions of shared_ptr types from an array new expression. modernize-make-shared looks for constructions of shared_ptr types using the new expression, However I'm not sure how it handles the array version.
I found out that make_shared can not handle array at all (until C++20, but I do not plan to handle such new features). modernize-make-shared should not transform into make_shared if an array is created (if it works correct) but I do not see test cases for this.
clang-tools-extra/clang-tidy/misc/SharedPtrArrayMismatchCheck.cpp | ||
---|---|---|
39 ↗ | (On Diff #399970) | The intent is to find cases where no deleter is specified. If a deleter is used we can not tell in reliable way if it is correct for an array. The check assumes that a deleter is correct and needs no check. |
104 ↗ | (On Diff #399970) | I think that this adds the additional range to the bug report. It is shown in the output (but I did not find a way how to check it in the test). |
106 ↗ | (On Diff #399970) | Yes, this is to check for "single variable declaration". But works only for variables (not fields). |
- Moved to bugprone module.
- Introduced a common base class, it is possible to add similar check for unique_ptr.
- Documentation changes.
Possible improvement: Add the check for reset too.
clang-tools-extra/test/clang-tidy/checkers/modernize-make-shared.cpp | ||
---|---|---|
48 ↗ | (On Diff #400791) | This change was not intentional (should be removed). |
clang-tools-extra/docs/clang-tidy/checks/bugprone-shared-ptr-array-mismatch.rst | ||
---|---|---|
23 | typo |
clang-tools-extra/clang-tidy/bugprone/SmartPtrArrayMismatchCheck.h | ||
---|---|---|
41 | There's only one implementation of this abstract method declared here. Why the extra level in the class hierarchy if there's only one implementation? |
clang-tools-extra/clang-tidy/bugprone/SmartPtrArrayMismatchCheck.cpp | ||
---|---|---|
42–48 | ||
clang-tools-extra/clang-tidy/bugprone/SmartPtrArrayMismatchCheck.h | ||
41 | The idea I think is so that this can be used to make a similar check for unique_ptr. However I almost don't see the need for this, and I'd argue that they should both be implemented in one check. So this will detect std::shared_ptr<int> X = new int[5]; std::unique_ptr<int> Y = new int[5]; |
clang-tools-extra/clang-tidy/bugprone/SmartPtrArrayMismatchCheck.h | ||
---|---|---|
41 | If I remember correctly the idea was to make the possible check for unique_ptr. Putting it into a separate check allows to independently turn the checks for shared or unique pointer on or off (like MakeSmartPtrCheck). |
There's only one implementation of this abstract method declared here.
Why the extra level in the class hierarchy if there's only one implementation?