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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 2 05:49:56 PDT 2019


aaron.ballman added inline comments.


================
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;
----------------
steakhal wrote:
> lebedev.ri wrote:
> > "*more* strictly aligned" perhaps?
> > Similarly, specify the actual values.
> Which metrics should I use? Bits or char units? For now I will stick to bits.
I tend to think in terms of bytes rather than bits as that's the allocation unit for storage. Might be worth adding "bytes" into the diagnostic as well, just to be very clear.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:41
+  const ASTContext &Context = *Result.Context;
+  const auto *castExpr = Result.Nodes.getNodeAs<CastExpr>("cast");
+  const bool HasCXXStyle = isa<CXXReinterpretCastExpr>(castExpr);
----------------
Should be renamed to meet coding style guidelines (but don't reuse `CastExpr` please, as that's a type name).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:42
+  const auto *castExpr = Result.Nodes.getNodeAs<CastExpr>("cast");
+  const bool HasCXXStyle = isa<CXXReinterpretCastExpr>(castExpr);
+
----------------
Please drop top-level `const` on non-pointer/reference automatics.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:47-50
+  if (!SrcType->isPointerType() || !DestType->isPointerType())
+    return;
+
+  if (SrcType->isDependentType() || DestType->isDependentType())
----------------
Can these checks be hoisted into the AST matcher?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:56
+
+  if (SrcPointedType->isIncompleteType() || DestPointedType->isIncompleteType())
+    return;
----------------
This one too.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:60
+  // -Wcast-align already warns for C-style casts increasing alignment
+  // requirements
+  const CharUnits SrcWidth = Context.getTypeSizeInChars(SrcPointedType);
----------------
Missing full stop at the end of the comment.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:65
+    diag(castExpr->getBeginLoc(),
+         "cast from %0 to %1 may lead to access memory based on invalid memory "
+         "layout; new type is wider (%2) than the original type (%3)")
----------------
I'd drop the "lead to" from the diagnostic text.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:74
+  // -Wcast-align already warns for C-style casts increasing alignment
+  // requirements
+  const CharUnits SrcAlign = Context.getTypeAlignInChars(SrcPointedType);
----------------
Missing full stop. This comment suggests that we should not be diagnosing in C mode because it's a duplicate diagnostic, but you're gating it on a config flag rather than then language mode. Is that intentional?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:79
+    diag(castExpr->getBeginLoc(),
+         "cast from %0 to %1 may lead to access memory based on invalid "
+         "memory layout; new type is more strictly aligned (%2) than the "
----------------
Same comments here as above regarding "lead to" and bytes vs bits.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:88
+
+  // make sure both pointee are structs
+  if (!SrcPointedType->isStructureType() || !DestPointedType->isStructureType())
----------------
Comments should be full sentences with proper capitalization and punctuation.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:104
+    const FieldDecl &DestField = **DestIterator;
+    if (SrcField.getType() != DestField.getType()) {
+      FieldsAreSame = false;
----------------
Should this be looking at the canonical types as opposed to whatever types are written? e.g., will this decide that a field of type `int` and one of type `int32_t` are different (assuming that `int32_t` is a typedef to `int`)?

What if the fields have the same type, but have different bit widths? e.g., `int i : 2;` vs `int i : 4;`?

Should this consider qualifiers, like _Atomic() for differences, or is that already covered by the type comparison?

Should structure packing or alignment attributes impact this?

What if the fields are the same but the layout is still different? e.g., `struct S { int i; };` vs 'struct S { virtual ~S() = default; int i; };` ?

What if one record is a struct and the other is a union, but their fields are the same? e.g., `struct S { int i; double d; };` vs `union U { int i; double d; };` ?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:111
+  if (!FieldsAreSame) {
+    const auto *SrcRecDecl = (*SrcIterator)->getParent();
+    const auto *DestRecDecl = (*DestIterator)->getParent();
----------------
Do not use `auto` here as the type is not spelled out in the initialization.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:115
+    diag(castExpr->getBeginLoc(),
+         "cast from %0 to %1 may lead to access memory based on invalid "
+         "memory layout; struct members are incompatible")
----------------
Drop "lead to" (same below). I'm not certain "invalid layout" is a good way to phrase it -- I think "incompatible layout" works better.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:6
+
+Warns for cases when pointer is casted and the destination type is wider than the
+original type.
----------------
aaron.ballman wrote:
> Can you re-flow this paragraph?
Warns for cases when a pointer is cast


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:6-11
+Warns for cases when pointer is casted and the destination type is wider than the
+original type.
+For example `char` vs `int`, `long` vs `char` etc.
+Also warns for cases when the layout of the destination type is different from the
+original one, like different structs, `int` vs `float`/`double`,
+different signedness.
----------------
Can you re-flow this paragraph?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:11
+original one, like different structs, `int` vs `float`/`double`,
+different signedness.
+
----------------
I don't think signedness is covered by the check (nor should it be, IMO).


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:13
+
+Allows pointer casts if the destination type is "part" of the original type.
+Which means the original type contains the members of the destination type
----------------
I think this could be reworded better in terms of common initial sequence (http://eel.is/c++draft/class.mem#def:common_initial_sequence), WDYT?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:17
+
+Highly recommended to use `-Wcast-align` whith this check which will cover some
+other cases of alignment requirement violations via casts.
----------------
It is highly recommended

whith -> with


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:25
+
+  This option can be configured to do not warn  when reinterpret cast is used.
+  Disabled by default but this option might be useful on code bases where 
----------------
to do not warn -> to not warn
(also, remove the extraneous whitespace after `warn`).

reinterpret cast -> reinterpret_cast (and add backticks)


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-incorrect-pointer-cast.rst:30
+Examples
+-------
+
----------------
Underlining looks too short here.


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

https://reviews.llvm.org/D48866





More information about the cfe-commits mailing list