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:
> 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.
>
>
>
Thanks! I'll stick with what I have for now, but `ImmutableMap` definitely
seems like a strict improvement, so I'll look into that next.
Regarding the representation -- I think I'd prefer subclassing over
typeclasses, but this sounds like a good discussion to have. It would probably
benefit us all to have a issue component to track ideas like this. I'll look
into the status of LLVM's move to github for issue tracking to see if there's a
way to set this up.
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
