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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:101-102
 // intended for API modeling that is not controlled by the target triple.
 def APIModeling : Package<"apiModeling">, Hidden;
 def GoogleAPIModeling : Package<"google">, ParentPackage<APIModeling>, Hidden;
 
----------------
I believe we want to put this checker as well as the return value checker into `apiModeling.llvm`.

This doesn't prevent the checkers from being generally useful for other packages: if we want to add support for other APIs, we'll simply reserve a different checker name for the same checker object (with internal flags to enable/disable particular API groups).


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:279
+  HelpText<"Model implementation of custom RTTIs">,
+  Documentation<NotDocumented>;
+
----------------
Szelethus wrote:
> This checker only does modeling, but isn't hidden. Should we hide it?
Dunno :)

I think we should hide the checkers that model core language functionality (including standard library functionality, as defining your own stuff in namespace std has undefined behavior in most cases, at least in c++) but we shouldn't hide checkers for specific user-made APIs because people may want to disable them simply because they have their own conflicting API.

Also, btw, the whole `apiModeling` package is currently hidden.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:61
+
+  return Ty.getAsString();
+}
----------------
Charusso wrote:
> NoQ wrote:
> > 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.
> Well, at least it does not crash. Thanks! I like that general return which we could evolve further. It is the same story like `Assuming...`.
No, it's not the same story. It's not "some valid situation that we don't support yet". It's "a normally impossible situation that indicates that we should not try to model this function because there's no reason for us to believe we know what it does".

(yay, i understood what you mean without a reference; that was hard^^)


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:1
+//===- CastValueChecker - Applies casts -------------------------*- C++ -*-===//
+//
----------------
Let's mention RTTI here as well. Otherwise it sounds weird, as all //actual// casts are applied by `ExprEngine`.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp:9
+//
+// This defines CastValueChecker which models casts.
+//
----------------
And here.


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

https://reviews.llvm.org/D64374