xazax.hun accepted this revision.
xazax.hun added inline comments.
This revision is now accepted and ready to land.
================
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;
----------------
ymandel wrote:
> 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.
Alternatively, I was wondering if deriving from `llvm::DenseMap<Key,
ElementLattice>` would reduce the amount of boilerplate this needs.
I'm a big fan of the typeclass approach, so I'd be really happy if the
framework supported something like that but it is definitely out of scope for
the PR.
> I think ultimately we'll want functional data structures, because the current
> setup forces an absurd amount of copying.
+1.
> 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.
Since we do not consider this to be the final/definitive solution, I'm fine
with either approach or leaving this as is.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116369/new/
https://reviews.llvm.org/D116369
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
