[llvm] r226746 - [canonicalization] Refactor how we create new stores into a helper

[Chandler Carruth]

On Wed, Jan 21, 2015 at 7:27 PM, Pete Cooper <peter_cooper at apple.com> wrote:

> This is more of a general question, but your patch is a good example (This
> shouldn’t block committing BTW).
>
> You’ve passed in InstCombiner &IC but only ever use it for IC.Builder.  Do
> you have a feel for when we should pass in a variable such as IC vs just
> pass in the bit we want like IC.Builder?  Makes the caller slightly more
> complex, but shows the scope of what we’re actually using.
>

I totally think this is an anti-pattern. I'm a bit embarressed to be
spreading it, but oh well.

So, my principled answer would be: we should definitely be passing the
builder, or we should be making a private method of InstCombiner.

In this specific case, I wasn't planning to only use the builder. And
indeed I'm about to submit a patch that uses DataLayout as well. What I
actually wanted was a private method on InstCombiner. I think that's what
roughly everyone wants here. Unfortunately, adding a private method means
going up to the header and adding another line, blah blah blah. I think
that's why this pattern pervades. That doesn't make it good, and we should
try to develop some reasonable practices that avoid it. I just don't have
those yet. I'll think more about what I think the right patterns would be,
as there are actually a *lot* of choices.


>
> This is perhaps more interesting in the backend where I think i’ve seen a
> bunch of functions take a MachineFunction when they just needed something
> like MachineRegisterInfo from the MF.
>

Agreed. I see it all over the place.

The worst offender is Sema in Clang. It is the most egregious and amazing
example of this pattern I have ever seen.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150121/456abaf8/attachment.html>