[PATCH] D67156: [Analyzer] Debug Checkers for Container and Iterator Inspection

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 6 inline comments as done.
baloghadamsoftware added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Checkers/Checkers.td:1328-1337
+def ContainerInspectionChecker : Checker<"ContainerInspection">,
+  HelpText<"Check the analyzer's understanding of C++ containers">,
+  Dependencies<[IteratorModeling]>,
+  Documentation<NotDocumented>;
+
+def IteratorInspectionChecker : Checker<"IteratorInspection">,
+  HelpText<"Check the analyzer's understanding of C++ iterators">,
----------------
Szelethus wrote:
> NoQ wrote:
> > Dunno, i would keep it all in one checker, just to save a few `// RUN:` lines :)
> I agree. Let's combine these into `DebugIteratorModeling`, because, as I understand it, that is what we're doing!
I did it on the short term, but on the long term iterator and container modelling are planned to be two different things, where iterator modelling depends on container modelling, but the latter works alone as well to support container checkers, such as e.g. copying data into a container with insufficient size. Furthermore I do not really like the name `debug.DebugIteratorModeling`.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:243-244
+                                  Getter get) const;
+  void analyzerContainerBegin(const CallExpr *CE, CheckerContext &C) const;
+  void analyzerContainerEnd(const CallExpr *CE, CheckerContext &C) const;
+  template <typename Getter>
----------------
NoQ wrote:
> We usually define such getters for stuff that the programmer cannot obtain otherwise during normal program execution. These two functions look like they're probably equivalent to normal `.begin()` and `.end()` calls. I don't really object but do we really ever need them other than for testing the trivial implementations of `.begin()` and `.end()`?
Not exactly. These functions return the internal representation of their `.begin()` and `.end()`. These symbols are conjured by the iterator checker and are bound to the return value of `.begin()` and `.end()` in the container data map. It is an extra check to check whether they return the same internal symbol as `clang_analyzer_iterator_position()` for the return value of `.begin()` and `.end()`.


================
Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:1689
+  if (CE->getNumArgs() == 0) {
+    reportDebugMsg("Missing container argument", C);
+    return;
----------------
Szelethus wrote:
> Ah, right, so this would fire for `clang_analyzer_iterator_position()`?
Yes, and also for the other two.


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

https://reviews.llvm.org/D67156