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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
