<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, May 2, 2016 at 12:55 PM Justin Bogner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>> writes:<br>
>> On May 2, 2016, at 11:20 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>> wrote:<br>
>><br>
>> Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>> writes:<br>
>>> Davide Italiano <<a href="mailto:dccitaliano@gmail.com" target="_blank">dccitaliano@gmail.com</a>> writes:<br>
>>>> Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@apple.com</a>> writes:<br>
>>>>> ================<br>
>>>>> Comment at: include/llvm/Transforms/IPO/GlobalDCE.h:44<br>
>>>>> @@ +43,3 @@<br>
>>>>> +  bool RemoveUnusedGlobalValue(GlobalValue &GV);<br>
>>>>> +};<br>
>>>>> +<br>
>>>>> ----------------<br>
>>>>> Can we split what is the "pass" and what is the actual utility?<br>
>>>><br>
>>>> I think it's possible, but maybe overkill? Probably people with more<br>
>>>> experience porting passes can tell what they think.<br>
>>><br>
>>> I think this should be the standard layout, it should be the exception<br>
>>> to merge together the utility logic and the pass logic.<br>
>><br>
>> Why? One of the nice things about the new pass manager design is that<br>
>> the pass *is* a utility. They're just objects that can do the transforms<br>
>> you care about and that happen to have a passmanager-compatible run<br>
>> method.<br>
><br>
> It does not "happen to have", it *forces you* to have.  If it just<br>
> "happened to have" the right interface, there shouldn't be an include<br>
> for the PassManager.<br>
<br>
Well, the interface uses the PreservedAnalyses and AnalysisManager<br>
types, that are defined in the PassManager include, so it's pretty hard<br>
to have the right interface without it...<br>
<br>
><br>
>><br>
>> I don't see how adding an extra layer of indirection would make things<br>
>> better, in the general case.<br>
><br>
> single responsibility principle, in the general case...<br>
<br>
I'm not convinced having a single utility that is a pass violates SRP.<br>
The class does one thing - the transformation. There may be multiple<br>
interfaces in terms of how it's called, but that's a different thing.<br>
<br>
OTOH, adding a layer of abstraction for no reason falls afoul of the<br>
"You aren't gonna need it" adage. It's building something we don't have<br>
a use for.<br>
<br>
In some cases breaking these up into multiple pieces will certainly make<br>
sense and make for a better design, but for simple passes like this it's<br>
a waste of time.<br></blockquote><div><br></div><div>FWIW, I tend to agree with Justin here.</div><div><br></div><div>I think the feeling I have about when this tips in favor of factoring things out is when there are multiple different usage scenarios, especially if that motivates us to have a completely non-pass utility API in a header like Transforms/Utils does.</div><div><br></div><div>-Chandler</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>