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