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

[Pete Cooper]

> On Jan 21, 2015, at 7:35 PM, Chandler Carruth <chandlerc at google.com> wrote:
> 
> 
> On Wed, Jan 21, 2015 at 7:27 PM, Pete Cooper <peter_cooper at apple.com <mailto: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.
I think we’ve all done it.  Shame on us all ;)
> 
> 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.
In this particular case i’d be tempted to get the DL from the builder, and if thats not an option we should probably add it (aren’t we getting it from the module these days anyways?).  But that can be another patch if its even useful.
>  
> 
> 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.
Might be worth a PR or two if anything really terrible jumps out (i’ll also try keep this in mind if i see really bad examples).  Could be good for a new developer to get used to llvm/clang with refactoring it to break this anti-pattern.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150121/a653d810/attachment.html>