[PATCH] D116369: [clang][dataflow] Add parameterized map lattice.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 3 inline comments as done.
ymandel added a comment.

In D116369#3212532 <https://reviews.llvm.org/D116369#3212532>, @sgatev wrote:

> It seems unnecessary to deal with AST elements in the tests for `MapLattice`. I think testing it with integer or string keys would be simpler. Given that `VarMapLattice` is just an alias, I don't think it's necessary to add dedicated tests for it. What do you think?

Yes, excellent point. I've stripped all of that out and think its much cleaner this way. Thanks!



================
Comment at: clang/include/clang/Analysis/FlowSensitive/MapLattice.h:33
+/// constructor of `ElementLattice` should produce the bottom element.
+template <typename Key, typename ElementLattice> class MapLattice {
+  using Container = llvm::DenseMap<Key, ElementLattice>;
----------------
sgatev wrote:
> Please document the relation, the join operation, and the bottom element for this lattice.
I wasn't sure which relation you had in mind, so documented the equality operation.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MapLattice.h:50
+  void insert(const std::pair<const key_type, mapped_type> &P) {
+    (void)C.insert(P);
+  }
----------------
sgatev wrote:
> Is `(void)` necessary? Is it a convention?
not necessary -- I'll sometimes do it when I'm ignoring a result. But, no need -- i dropped it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116369