[PATCH] D64374: [analyzer] CastValueChecker: Model casts

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a subscriber: rnkovacs.
NoQ added a comment.

We should add at least some tests for the other three functions.



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:278
+def CastValueChecker : Checker<"CastValue">,
+  HelpText<"Model casts">,
+  Documentation<NotDocumented>;
----------------
"Model implementation of custom RTTIs."


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h:250
 
+  /// A shorthand version of getNoteTag that accepts a plain note.
+  ///
----------------
Great! I recognize the need for such function now.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:25-27
+  using CastCheck =
+      std::function<void(const CastValueChecker *, const CallExpr *,
+                         DefinedOrUnknownSVal, CheckerContext &)>;
----------------
Charusso wrote:
> NoQ wrote:
> > Kinda nice, but a simple pointer-to-member would have been sufficient and much more lightweight:
> > 
> > ```lang=c++
> > using CastCheck = void (CastValueChecker::*)(const CallExpr *,
> >                          DefinedOrUnknownSVal, CheckerContext &);
> > ```
> > 
> > (not sure, i don't fully understand the performance implications of using `std::function`)
> It took me an hour to realize I have to push the class as the signature of the function to create a member-callback. I think it is not that lightweight, but it is modern. I believe it is made for that purpose when you could write `Foo[13]` or `*(13 + P)`, where the latter creeps me out.
> 
> Thanks for the copy-pasteable code, I really enjoy to see what do you think exactly, but this is the first time when I would pick my idea. (I would had recommended that modern way in your patch just I thought it is not working with methods.)
Ok then!


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:58-59
+  QualType Ty = Cast->getType();
+  if (const CXXRecordDecl *CD = Ty->getPointeeCXXRecordDecl())
+    return CD->getName();
+
----------------
I suspect that this may crash if `CD` is an anonymous structure. So you should use `getNameAsString()` instead.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:61
+
+  return Ty.getAsString();
+}
----------------
Use after free! `QualType::getAsString()` returns a temporary `std::string`. You're returning a `StringRef` that outlives the string it refers to. The solution is usually to return the `std::string` by value.

//*summons @rnkovacs*//

Generally, i'd rather bail out on this branch. If we're seeing a dyn_cast of something that //isn't a class//, we're already doing something wrong.


================
Comment at: clang/test/Analysis/cast-value.cpp:3
+// RUN:  -analyzer-checker=core,apiModeling.CastValue,debug.ExprInspection \
+// RUN:  -verify=logic %s
+// RUN: %clang_analyze_cc1 \
----------------
Nice!

I guess putting them into separate files would have been easier this time.


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

https://reviews.llvm.org/D64374