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

[Stanislav Gatev via Phabricator via cfe-commits]

sgatev added a comment.

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?



================
Comment at: clang/include/clang/Analysis/FlowSensitive/MapLattice.h:1
+//===- DataflowAnalysis.h ---------------------------------------*- C++ -*-===//
+//
----------------
The comment needs to be updated.


================
Comment at: clang/include/clang/Analysis/FlowSensitive/MapLattice.h:15
+
+#ifndef LLVM_CLANG_ANALYSIS_FLOWSENSITIVE__MAPLATTICE_H_
+#define LLVM_CLANG_ANALYSIS_FLOWSENSITIVE__MAPLATTICE_H_
----------------
Remove the last underscore for consistency.


================
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>;
----------------
Please document the relation, the join operation, and the bottom element for this lattice.


================
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);
+  }
----------------
Is `(void)` necessary? Is it a convention?


================
Comment at: clang/unittests/Analysis/FlowSensitive/MapLatticeTest.cpp:12-13
+
+namespace clang {
+namespace dataflow {
+namespace {
----------------
```
using namespace clang;
using namespace dataflow;
```


================
Comment at: clang/unittests/Analysis/FlowSensitive/MapLatticeTest.cpp:22
+MATCHER_P(Var, name,
+          (llvm::Twine(negation ? "isn't" : "is") + " a variable named `" +
+           name + "`")
----------------
`#include "llvm/ADT/Twine.h"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116369