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

[Pete Cooper]

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.

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.

Thanks,
{ete
> On Jan 21, 2015, at 6:55 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
>> 
>> On 2015-Jan-21, at 18:23, Chandler Carruth <chandlerc at gmail.com> wrote:
>> 
>> 
>> On Wed, Jan 21, 2015 at 6:18 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>> On 2015-Jan-21, at 16:39, Chandler Carruth <chandlerc at gmail.com> wrote:
>>> 
>>> 
>>> On Wed, Jan 21, 2015 at 3:45 PM, Chandler Carruth <chandlerc at gmail.com> wrote:
>>> [canonicalization] Refactor how we create new stores into a helper
>>> function. This is a bit tidier anyways and will make a subsquent patch
>>> simpler as I want to add another case to this combine.
>>> 
>>> I didn't end up needing this, but it seems cleaner to leave it in. Let me know if anyone prefers the old code.
>> 
>> New code looks better to me.  Might be cleaner without the return value
>> if you're not using it, though.
>> 
>> I lied, and will end up using it after all. Just need to move it in my next commit.
>> 
>> I'm still not using the return value, but it felt weird to not return it. I can definitely imagine a caller in the future wanting it, so just wanted to make the utility function useful to them if/when they arrive as it adds essentially no complexity.
>> 
>> Still, I'm happy to remove it if that's your preference. Just let me know.
> 
> Whatever :).  Looks like dead code (and a new caller could easily add
> it back), but it's harmless, and I agree it looks kind of weird
> without it.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150121/8715f93f/attachment.html>