<div dir="ltr">Shankar,<div><br></div><div>This is to revert the patch you submitted that you thought would fix some issue with RoundTripYAMLPass but turned out that the assumption was wrong and actually introduced a regression. As Chandler and Reid wrote, such patch would usually be reverted immediately and then start over if necessary. Comments to the original code would be appreciated, but that shouldn't block the rollback.</div>



<div><br></div><div>It was unfortunate that there's no test that covers this case. Once we enable RoundTripYAMLPass for PECOFF, the pass would catch the error, thanks to your new test pass. :) I don't see an immediate need to write a test for this, as I'm working hard on resolving remaining issues with RoundTrip passes. Hopefully we'll be able to enable it soon.</div>



<div><br></div><div>That being said, I'm fine to add a new assertion to catch this error as a temporary measure until RoundTrip tests are enabled. That new check shouldn't be mixed with a rollback, but I'm OK to do that too to avoid push-back. I'll update the patch.</div>

<div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 30, 2013 at 6:35 PM, Shankar Easwaram <span dir="ltr"><<a href="mailto:shankarke@gmail.com" target="_blank">shankarke@gmail.com</a>></span> wrote:<br>


<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">It became more of a discussion post this revert as I was unclear on how the atom got inserted into the file twice and all tests were still passing. I think there is a lack of test otherwise this could have been caught earlier.<br>



<br>
We should add a test along with the revert to make sure the revert actually fixes something.<br>
<br>
Sent from my iPhone<br>
<div><div><br>
> On Oct 30, 2013, at 19:32, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br>
><br>
><br>
>  Also, if you want to change the design of how atoms get added, I really don't see why that should happen in this change.<br>
><br>
>  It seems much better to revert a change which introduced a regression, and *then* consider new or different designs that would be simpler or more efficient for some reason.<br>
><br>
> <a href="http://llvm-reviews.chandlerc.com/D2069" target="_blank">http://llvm-reviews.chandlerc.com/D2069</a><br>
</div></div></blockquote></div><br></div></div>