[PATCH] D91948: [WIP][analyzer][doc] Add Container- and IteratorModeling developer docs

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.
Herald added a subscriber: manas.

Nice work!



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:125
+The begin and end symbols are conjured and are completely unrelated to the region of
+the container. For each region we store the only the begin and end symbols, other properties
+are to be computed from these, and their relationships stored in the ContstraintManager.
----------------



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:148
+lvalue see `value categories <https://en.cppreference.com/w/cpp/language/value_category>`_)
+modeling is not prominent is case containers alone, as no temporary objects are considered.
+However, this is an issue to be solved when it comes to modeling iterators.
----------------
whisperity wrote:
> I think something is missing for here, I am incapable of decyphering this sentence.
+1


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:160
+could not reorder expressions containing the the symbol. As an orthogonal issue symbol-symbol
+comparisons still cannot be handled properly if the ``ContstraintManager`` would also be able
+answer questions like: is symbol A less than symbol B (instead of just reporting the possible
----------------
Or `...comparisons still cannot be handled properly. If the ``ContstraintManager`` was also able ... then ...


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:210
+ - only track an iterator if its generating expression is a function call which has at least 1 argument, that is an already tracked iterator (and the first iterator parameter's container will be the parent container of the returned iterator)
+If either of these heuristics matches the tracking of iterator should be skipped.
+
----------------
whisperity wrote:
> Should be?
> Is.



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:222
+
+The iterator offset is abstract, no ``MemRegionVal`` is associated with iterator offsets.
+
----------------
`:`


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:227
+
+Functions like find (when alpha.cplusplus.STLAlgorithmModeling is enabled) handle cases where an
+element is found, and a case where it is not
----------------
whisperity wrote:
> 



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:264
+    int* it = c.begin(); // container-modeling begins by tracking c as a container of type Cont
+                         // begin() member function call triggers the modeling
+                         // iterator-modeling also begins by tracking the value of c.begin()
----------------



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:266
+                         // iterator-modeling also begins by tracking the value of c.begin()
+                         // as an iterator
+                         // we check if the value has the necessary iterator-properties
----------------



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:276
+                            // position, noting it in the modeling structure for c as end position
+                            // comparion operator== triggers a state-split, branch a assuming that
+                            // it position is equal to the newly created end position, branch b
----------------
branch 'A'


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:290
+
+Contrary to the container-modeling, not only lvalue iterators are tracked. This is the reason
+why 2 different keys are used in the GDM for iterators. An lvalue iterator has a Region
----------------
Szelethus wrote:
> whisperity wrote:
> > whisperity wrote:
> > > 
> > Previously, `lvalue` was written as `LValue`!
> Ah, there it is. Move it to where I placed my other inline! :) 
Why don't we track Rvalue containers? It's not clear from this document.


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:310-313
+Iterators implemented as pointer live generally in the SymbolMap (the map containing ``SymbolRef``-s as
+opposed to the map containing the ``const MemRegion*``-s), and can not be only represented with LValues (and
+consequently inside the RegionMap), as tracking the value of symbolic offset of an iterator must handle
+expressions where there may only be temporaries with no LValues associated with them.
----------------
Break up this sentence please.


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:317
+because pointer iterators have only Rvalues that always identifies them (across multiple subexpressions),
+class instance variables only have Lvalues for this role. SymbolMap always has iterator values that are RValues.
+RegionMap can have iterator values which are LVals but also values which are RValues.
----------------
whisperity wrote:
> 
I think it would be useful to have a few sentences (preceding this paragraph) that introduces the `SymbolMap` and `RegionMap` and for what they used for.


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:329
+that accessing a pointers ``SVal`` always return reference to a Region (no way to be a symbol, SymRegion),
+but in case if a class instance iterator can be a symbol (SymExpr).
+
----------------



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:353
+  straightforward way to manage the symbolic values of iterators is essential to effectively progress with the 
+  implementation. Also the case of nested iterator modeling (where there is a container which has iterator
+  as elements) the iterators that iterate a container and the iterators contained in the said container must be
----------------
`iterators`


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:369
+  Class instance iterators that evaluate as a result of multiple subexpressions have RValues and these immediate
+  RValues break a cheain of value propagation. lazyCompoundVal should not be used as keys in a map (every
+  operation results in a temporary which can be tracked).
----------------
whisperity wrote:
> whisperity wrote:
> > 
> typo
`chain`


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:369
+  Class instance iterators that evaluate as a result of multiple subexpressions have RValues and these immediate
+  RValues break a cheain of value propagation. lazyCompoundVal should not be used as keys in a map (every
+  operation results in a temporary which can be tracked).
----------------
martong wrote:
> whisperity wrote:
> > whisperity wrote:
> > > 
> > typo
> `chain`



================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:371-372
+  operation results in a temporary which can be tracked).
+  Pointer iterators that evaluate as a result of multiple subexpressions have RValues and these immediate RValues break
+  the chain of value propagation. lazyCompoundVal should not be used as keys in a map (every operation results in a
+  temporary which can be tracked). no temporaries are created during the evaluation of expressions (i + 1 + 2) there
----------------
These sentences are word-by-word copies of the previous two sentences.


================
Comment at: clang/docs/analyzer/developer-docs/ContainerModeling.rst:380
+  The drawback of this approach is that the implementation for modeling the 2 families of iterators are harder to
+  share, which either leads to duplication or an extra layer of abstraction.
+  The ability of LazyCompoundVal to take the role of the key inside the map (ordering or hashing) should still be
----------------
I vote for the extra layer of abstraction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91948