<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Sun, Aug 30, 2015 at 3:13 AM Renato Golin 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">On 30 August 2015 at 03:09, Chandler Carruth via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: chandlerc<br>
> Date: Sat Aug 29 21:09:48 2015<br>
> New Revision: 246367<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=246367&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=246367&view=rev</a><br>
> Log:<br>
> [Triple] Stop abusing a class to have only static methods and just use<br>
> the namespace that we are already using for the enums that are produced<br>
> by the parsing.<br>
<br>
Hi Chandler,<br>
<br>
So, about this one, I wish you had asked first.<br></blockquote><div><br></div><div>This is a very simple change, and no one seemed actively modifying it, so I don't think there is a problem here...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
This is a big mess, yes, but the answer is not that simple. It did<br>
occur to me to do it like that from the beginning, you'd be glad to<br>
know, but there are further design issues that stopped me from doing<br>
it, namely future inheritance when we get to a more generic target<br>
parsing.<br></blockquote><div><br></div><div>I can't imagine how inheritance is any part of a correct design for a text parsing solution here.</div><div><br></div><div>Even if there are some future plans, we shouldn't design the code in a way that makes no sense today because we think (without evidence) that it will make more sense in the future. YAGNI, or insert your software design aphorism here.</div><div><br></div><div>This code, today, is doing procedural text parsing, and as such should just be using functions in a name space. If and when some future design is actually proposed, I think that would be the time to change it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Could you please revert your changes on TargetParser until we can all<br>
agree on an implementation that makes sense to the future and present?<br></blockquote><div><br></div><div>I don't think that is the correct approach. This patch in particular I feel *extremely* confident is a strictly positive step forward.</div><div><br></div><div>We should not revert to a worse state while we figure out some hypothetical better state.</div><div><br></div><div>Now if you want to remove *all* of this code until that better state is identified, that makes more sense but I think that ship has sailed. That would be perceived as an acceptable regression. The only path forward I see here is to make the code that we have today be reasonable, and ask you and others to try to come up with a better design long term. I think these patches are reasonable steps in the first goal.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
cheers,<br>
--renato<br>
_______________________________________________<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>