[PATCH] Revert "[PECOFF] Add atoms to the PassManager file"

[Rui Ueyama]

Shankar,

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.

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.

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.

On Wed, Oct 30, 2013 at 6:35 PM, Shankar Easwaram <shankarke at gmail.com>wrote:

> 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.
>
> We should add a test along with the revert to make sure the revert
> actually fixes something.
>
> Sent from my iPhone
>
> > On Oct 30, 2013, at 19:32, Chandler Carruth <chandlerc at gmail.com> wrote:
> >
> >
> >  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.
> >
> >  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.
> >
> > http://llvm-reviews.chandlerc.com/D2069
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131030/3b36e098/attachment.html>