[PATCH] D105552: [analyzer][NFC] NoStoreFuncVisitor: Compact parameters for region pretty printing into a class

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

Honestly, I don't really see how this is better.
IMO `Printer` is something that prints, it should be everything that it does.  It can accept different parameters tweaking how it prints in its constructor, but if it is a region printer, you should give it a region, and it should print it.  It's not a one-use thing.

Here it looks like you aggregate all the information about the region inside of this class, and it knows how to print it.  And it looks like this is actually a primary reason here, you want a good way to aggregate all these million parameters into one single object.  A better name (and purpose) for abstraction like this would be to actually make all these parameters to make sense together.  So that the visitor produces them together and then we process them together.  And one nice property of such aggregated result would be ability to print it.

Another problem that I see is that even for such a small class, I have no idea how to use it.  It has too many parameters and it doesn't tell me what is the relationship between them.  My guess is that in this form this class will never be reused outside of `NoStoreFuncVisitor`, then why do we need it?

At this point, I don't see how this abstraction adds any meaningful layering.  Meaningful abstraction defines a language for a problem that we are trying to solve, so we can use it naturally when we talk.

Sorry about being pretty harsh here.  I do think that `NoStoreFuncVisitor` needs refactoring and better abstractions, I just don't think that this is the way to go.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105552