[PATCH] D93762: SCCP: Refactor SCCPSolver

[Sanne Wouda via Phabricator via llvm-commits]

sanwou01 added a comment.

I think the issue here is that quite a number of implementation details are leaking into the header file where they don't belong because they are not part of the public interface. You're right that code using the header file doesn't have access to these by virtue of being private, but this does not make for a very readable header file, IMO.

In this case, SCCPSolver seems to carry quite a bit of state, so it might make sense to split it into two classes, one in the cpp file with all the implementation details, e.g. inheriting from InstVisitor, containing all the DenseMap state, and implementing the private member functions, and one class in the header file which can be a thin wrapper around the first class. Transform/Utils/ValueMapper.h takes a similar approach and might be a good example.

Does that make sense? @fhahn, is that roughly what you had in mind?


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

https://reviews.llvm.org/D93762