<div dir="ltr">Folks, I want to be super, super clear here.<div><br></div><div>It is really not OK to take some previous patch that you didn't author, take it over, and mail it out as a fresh patch. That is not how Open Source works and that is not how LLVM works.</div><div><br></div><div>You need to attribute that patch to the author. You need to follow up with the original authors and the original reviewers.</div><div><br></div><div>I'm particularly concerned because the previous approach had a *documented* problem that both the author and George discussed in detail. This patch should never have been committed (regardless of who says LGTM) until those concerns have been discussed and addressed (or the folks with those concerns found to be unavailable on completely unresponsive).</div><div><br></div><div>I'm calling this out here because this is super important to get right in order to be an effective contributor to this (and most other) open source projects.</div><div><br></div><div>Please *explicitly* and carefully document where this patch came from. Give credit to Jia Chen who wrote the original patch. Discuss the delta (as you are starting too, but please be more explicit), and make sure at least George (the reviewer of the previous patch and owner of CFLAA) is happy, and make *some* attempt to loop Jia Chen back in to cover any remaining concerns there.</div><div><br></div><div>I *am* really happy to see folks picking up efforts that got stalled, and helping out with them. That is a great pattern we should do more of in LLVM. But we have to do it the right way, and make sure people are aware and involved at the right points.</div><br><div class="gmail_quote"><div dir="ltr">On Thu, May 24, 2018 at 4:24 PM George Burgess IV via Phabricator 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">george.burgess.iv added a comment.<br>
<br>
So, to be clear, you're saying that this is (essentially) exactly <a href="https://reviews.llvm.org/D22721" rel="noreferrer" target="_blank">https://reviews.llvm.org/D22721</a>, but rebased and with the extra check on line 822, yes?<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D46282" rel="noreferrer" target="_blank">https://reviews.llvm.org/D46282</a><br>
<br>
<br>
<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>