[PATCH] D58397: [analyzer] MIGChecker: Pour more data into the checker.

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

Aah, MIG_NO_REPLY.

LGTM.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:42
+  std::vector<std::pair<CallDescription, unsigned>> Deallocators = {
+#define CALL(required_args, deallocated_arg, ...)                              \
+  {{{__VA_ARGS__}, required_args}, deallocated_arg}
----------------
Could you put a comment with an example indicating how CALL works for a particular example function? I think that will make is easier for folks to add support for new APIs in the future without getting it wrong.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:125
 
+  // See if there's an annotated method in the superclass.
+  if (const auto *MD = dyn_cast<CXXMethodDecl>(D))
----------------
Perhaps we could make it a Sema error if a method doesn't have a mig server routine annotation but it overrides a method that does. From a documentation/usability perspective it would be unfortunate if a human looking at a method to determine its convention would have to look at its super classes to see whether they have an annotation too.

Although, thinking about it more that might make it a source-breaking change if an API author were to add annotation on a super class. Then API clients would have to add the annotations to get their projects to build, which would not be great..


================
Comment at: clang/lib/StaticAnalyzer/Checkers/MIGChecker.cpp:162
+// Returns true if V can potentially represent a "successful" kern_return_t.
+static bool isSuccess(SVal V, CheckerContext &C) {
+  ProgramStateRef State = C.getState();
----------------
Could we rename this to "mayBeSuccess()" or something like that so that the name of function indicate the "potentially" part of the comment.


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

https://reviews.llvm.org/D58397