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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked an inline comment as done.
NoQ added inline comments.


================
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))
----------------
dcoughlin wrote:
> 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..
Yup, i believe that given that the checker is an optional thing (even if we make it opt-out the hard way), annotating APIs should also be optional. The primary use case for this override trick is the `IOUserClient::externalMethod()` API that gets annotated within IOKit itself and makes the attribute automatically apply to all overrides in all IOKit projects, automagically making them subject to testing. But if we make it an error to not annotate the overrides, it'd break every single IOKit project and require them to add dozens of annotations before it compiles, so i think it's not feasible.

We might as well hardcode the `IOUserClient::externalMethod()` thing (instead of annotating it) and in this case it would make much more sense to enforce fully annotating all overrides.


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

https://reviews.llvm.org/D58397