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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
