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

[Balázs Kéri via Phabricator via cfe-commits]

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117306/new/

https://reviews.llvm.org/D117306