[PATCH] D117306: [clang-tidy] Add new check 'shared-ptr-array-mismatch'.

Balázs Kéri via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Jan 17 04:04:18 PST 2022

balazske marked 2 inline comments as done.
balazske added a comment.

In D117306#3247859 <https://reviews.llvm.org/D117306#3247859>, @njames93 wrote:

> In D117306#3247417 <https://reviews.llvm.org/D117306#3247417>, @LegalizeAdulthood wrote:
>> In D117306#3245915 <https://reviews.llvm.org/D117306#3245915>, @njames93 wrote:
>>> How does this check play with the `modernize-make-shared` check?
>> 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.
> 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.

Comment at: clang-tools-extra/clang-tidy/misc/SharedPtrArrayMismatchCheck.cpp:39
+      cxxConstructExpr(
+          hasDeclaration(UsedConstructor), argumentCountIs(1),
+          hasArgument(0, cxxNewExpr(isArray(), hasType(pointerType(pointee(
LegalizeAdulthood wrote:
> What about the other constructor overloads?
> Creating a `shared_ptr` with a deleter is quite common.
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.

Comment at: clang-tools-extra/clang-tidy/misc/SharedPtrArrayMismatchCheck.cpp:104
+                                            .getLocalSourceRange();
+    D << TemplateArgumentRange;
LegalizeAdulthood wrote:
> I'm not sure what this is doing?
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).

Comment at: clang-tools-extra/clang-tidy/misc/SharedPtrArrayMismatchCheck.cpp:106
+    if (isInSingleDeclStmt(VarOrField)) {
+      const SourceManager &SM = Ctx.getSourceManager();
LegalizeAdulthood wrote:
> Is this to guard against multiple variables being declared at once?
Yes, this is to check for "single variable declaration". But works only for variables (not fields).

  rG LLVM Github Monorepo



More information about the cfe-commits mailing list