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

Thu Dec 30 05:32:07 PST 2021

ymandel marked an inline comment as done.
ymandel added inline comments.

================
Comment at: clang/include/clang/Analysis/FlowSensitive/MapLattice.h:92
+  /// entry as it was in the source map.
+  LatticeJoinEffect join(const MapLattice &Other) {
+    LatticeJoinEffect Effect = LatticeJoinEffect::Unchanged;
----------------
xazax.hun wrote:
> It looks like apart from the join operation the rest of the methods are simply forwarding to DenseMap. I was wondering if it would make more sense to make the framework support `join` as a free function (possibly using some custom type traits?) to avoid forcing the user to write wrappers like this.
Good point, but I think that these concerns are separable -- that is, how much forwarding we do and whether we should enable `join` an arbitrary types.

For the first issue, an alternative design here would be to simply expose the container as a public field and drop all the methods except for `join`.  I intentionally left some abstraction in place, though, because I think that `DenseMap` is not the right container, its just "good enough" to get started. I think ultimately we'll want functional data structures, because the current setup forces an absurd amount of copying.

For the second issue, I'm open to the idea -- it would be like Haskell's type classes in some sense, but I prefer the design where the lattice's operations are grouped together as a unit. I think that we could fit it into the current design with some form of SFINAE-based discrimination on the lattice type parameter of `DataflowAnalysis`.

Given that, what do you think of just renaming this to `DenseMapLattice` and exposing the container field publicly? When we're ready with a better map lattice, we can add that alongside this one with a different name.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116369