[PATCH] D60139: [clang-tidy] Add misc-placement-new-target-size check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:10
+#include <iostream>
+
+#include "PlacementNewTargetSizeCheck.h"
----------------
Unnecessary empty line. Please run Clang-format after fixing.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:23
+namespace {
+CharUnits getSizeOfType(clang::ASTContext *const Context, const QualType& type) {
+  CharUnits TypeSize;
----------------
Please use static instead of anonymous namespaces. See LLVM Coding Guidelines.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:26
+  if (type->isRecordType()) {
+    const auto *CXXRecordDecl = type->getAsRecordDecl();
+    assert(CXXRecordDecl && "type->getAsRecordDecl() failed");
----------------
Please don't use auto where type could not be easily deduced.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:28
+    assert(CXXRecordDecl && "type->getAsRecordDecl() failed");
+    const auto &CXXDeclLayout =
+        CXXRecordDecl->getASTContext().getASTRecordLayout(CXXRecordDecl);
----------------
Please don't use auto where type could not be easily deduced.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:47
+    const MatchFinder::MatchResult &Result) {
+  const CXXNewExpr *Alloc = Result.Nodes.getNodeAs<CXXNewExpr>("Alloc");
+  assert(Alloc && "Matched node bound by 'Alloc' shoud be a 'CXXNewExpr'");
----------------
auto could be used here, because of cast.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:50
+
+  auto numPlacementArgs = Alloc->getNumPlacementArgs();
+  if (0 == numPlacementArgs) {
----------------
Please don't use auto where type could not be easily deduced.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:55
+
+  auto type = Alloc->getAllocatedType();
+  CharUnits TypeSize = getSizeOfType(Result.Context, type);
----------------
Please don't use auto where type could not be easily deduced.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:62
+  // get ptr type of implicit cast
+  const ImplicitCastExpr *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("Cast");
+  auto CastType = Cast->getSubExpr()->getType();
----------------
auto could be used here, because of cast.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:63
+  const ImplicitCastExpr *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("Cast");
+  auto CastType = Cast->getSubExpr()->getType();
+  if (CastType->isPointerType()) {
----------------
Please don't use auto where type could not be easily deduced.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:65
+  if (CastType->isPointerType()) {
+   CharUnits CastSize = getSizeOfType(Result.Context, CastType->getPointeeType());
+    if ((TypeSize - CastSize).isPositive()) {
----------------
Please run Clang-format.


================
Comment at: clang-tidy/misc/PlacementNewTargetSizeCheck.cpp:73
+  }
+
+}
----------------
Unnecessary empty line.


================
Comment at: docs/ReleaseNotes.rst:135
+  <clang-tidy/checks/misc-placement-new-target-size>` check.
+
 - New :doc:`openmp-exception-escape
----------------
Please add one sentence description. Should be same as in documentation.


================
Comment at: docs/clang-tidy/checks/misc-placement-new-target-size.rst:5
+==============================
+
+
----------------
Unnecessary empty line.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D60139