[PATCH] D65889: [analyzer] CastValueChecker: Model castAs(), getAs()

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:26
 class CastValueChecker : public Checker<eval::Call> {
+  enum class CastKind { Checking, Sugar };
+
----------------
NoQ wrote:
> Please explain "Checking" and "Sugar". Checking what? Sugar... you mean syntactic sugar? For what?
I have no idea what devs mean by those names:
- `Checking` kind:
> The cast<> operator is a “checked cast” operation.
> The dyn_cast<> operator is a “checking cast” operation.
from http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

- `Sugar` kind:
> Member-template castAs<specific type>. Look through sugar for the underlying instance of <specific type>.
> Member-template getAs<specific type>'. Look through sugar for an instance of <specific type>.
from https://clang.llvm.org/doxygen/classclang_1_1Type.html#a436b8b08ae7f2404b4712d37986194ce
and https://clang.llvm.org/doxygen/classclang_1_1Type.html#adae68e1f4c85ede2d36da45fbefc48a2

- `isa()` would be `Instance` kind:
> The isa<> operator works exactly like the Java “instanceof” operator.
from http://llvm.org/docs/ProgrammersManual.html#the-isa-cast-and-dyn-cast-templates

If you could imagine better names, please let me know. I have tried to use the definitions.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:94
+      [CastFromName, CastToName, IsNullReturn,
+       IsSimpleCast](BugReport &) -> std::string {
         SmallString<128> Msg;
----------------
NoQ wrote:
> `IsDynamicCast`.
It is not `dyn_cast` but simple `cast`. Because we have decided to use `Checked` for that cast, I believe `IsCheckedCast` the one.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:98
 
-        Out << "Assuming dynamic cast from '" << CastFromName << "' to '"
-            << CastToName << "' succeeds";
-        return Out.str();
-      },
-      /*IsPrunable=*/true);
+        Out << (!IsSimpleCast ? "Assuming dynamic cast " : "Dynamic cast ");
+        if (CastFromName)
----------------
NoQ wrote:
> More suggestions for the `:`-branch: "Hard cast" (a bit too jargon-y), "Safe cast", "Checked cast".
Well, let us pick `Checked` then as that is the proper definition for `cast()`, but that other sugar-based `castAs()` definition makes me mad.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:220
 
-  // If we cannot obtain both of the classes we cannot be sure how to model it.
-  if (!CE->getType()->getPointeeCXXRecordDecl() ||
-      !CE->getArg(0)->getType()->getPointeeCXXRecordDecl())
-    return false;
+  const CastCheck *Check = &Lookup->first;
+  CastKind Kind = Lookup->second;
----------------
NoQ wrote:
> What's the point of making this a raw pointer? (no pun intended)
I just wanted to make it usable as it being used since the first review as `(*Check)` and emphasize it is not a given function-call but a pointer to one. If you think as a nude `Check()` it is fine, then I have no idea which is better and it is fine for me as well.


================
Comment at: clang/test/Analysis/cast-value.cpp:156-167
+void evalNonNullParamNonNullReturn(const Shape *S) {
+  const auto *C = cast<Circle>(S);
+  // expected-note at -1 {{Dynamic cast from 'Shape' to 'Circle' succeeds}}
+  // expected-note at -2 {{Assuming pointer value is null}}
+  // expected-note at -3 {{'C' initialized here}}
+
+  (void)(1 / !(bool)C);
----------------
NoQ wrote:
> Mmm, wait a sec. That's a false positive. `cast<>` doesn't accept null pointers. We have `cast_or_null` for this.
This `Assuming pointer value is null` note is very random. I believe it is not made by me and my code is fine, so I have printed a graph:
{F9759380}
Do you see any problem?


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

https://reviews.llvm.org/D65889