r215365 - Coverage mapping: emit mapping for cxx constructors that use microsoft's ABI

[Chandler Carruth]

On Mon, Aug 11, 2014 at 11:09 AM, Alex L <arphaman at gmail.com> wrote:

> Sorry, let me try to explain.
> I've commited a patch with the tests for coverage mapping on Friday with
> all the tests that failed on some buildbots and all of the windows
> buildbots.
>

OK


> I've resubmitted the patch today with all the tests, but 2 of the tests
> need this commit to pass on windows.
>

It would have made more sense to have a single commit with this
functionality and those two tests. =/ The idea of incremental development
is that you commit the minimum chunk of functionality and the tests for
that functionality in a single commit.


> All the tests are batched in a single patch because the code for coverage
> mapping generation was commited before the tests, and all of those tests
> are required for the generation.
>

Ok, please don't submit patches this way in the future. I'm sorry that
someone OK'ed the patch with the coverage mapping generation to be
committed, they were wrong to do so. We do not accept patches that add
functionality without tests, period.

If the code review for the tests is finished at this point, please commit
them with an "oops" and a pointer to the commit(s) with the functionality
they are testing.

However, if the tests are not ready to be committed (that is, the code
review is not finished) then I would suggest reverting your functionality
and merging it into the patch which contains the test cases for that
functionality. (And potentially splitting that into smaller chunks for
easier review.)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140811/52a6191a/attachment.html>