[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Some basic thoughts



================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:63
+         "cast from %0 to %1 may lead to access memory based on invalid memory "
+         "layout; pointed to type is wider than the allocated type")
+        << SrcPointedType << DestPointedType;
----------------
this reads weirdly, should be
"new type is wider (%2) than the original type (%3)"



================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:69-70
+         "cast from %0 to %1 may lead to access memory based on invalid "
+         "memory layout; pointed to type is strictly aligned than the "
+         "allocated type")
+        << SrcPointedType << DestPointedType;
----------------
"*more* strictly aligned" perhaps?
Similarly, specify the actual values.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:88
+      if (SrcField.getType() != DestField.getType() || SrcIterator == SrcEnd) {
+        FieldsAreSame = false;
+      }
----------------
`break;'?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:95
+           "cast from %0 to %1 may lead to access memory based on invalid "
+           "memory layout; struct members are incompatible")
+          << SrcPointedType << DestPointedType;
----------------
Would be better to at least point to first two incompatible fields


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:98-107
+  } else if (WarnForDifferentSignedness &&
+             ((SrcPointedType->isSignedIntegerType() &&
+               DestPointedType->isUnsignedIntegerType()) ||
+              (SrcPointedType->isUnsignedIntegerType() &&
+               DestPointedType->isSignedIntegerType()))) {
+    diag(castExpr->getBeginLoc(),
+         "cast from %0 to %1 may lead to access memory based on invalid "
----------------
What does this mean?
How can signedness affect memory layout?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h:18
+
+/// Warns for cases when pointer is cast and the pointed to type is incompatible
+/// with allocated memory area type. This may lead to access memory based on
----------------
is cast*ed*
s/pointed to/new/


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.h:19
+/// Warns for cases when pointer is cast and the pointed to type is incompatible
+/// with allocated memory area type. This may lead to access memory based on
+/// invalid memory layout.
----------------
How do you know anything about the actual `allocated memory area type`.
You probably want to talk about the original type before cast.


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

https://reviews.llvm.org/D48866