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

[Alex L]

2014-08-11 11:20 GMT-07:00 Chandler Carruth <chandlerc at google.com>:

>
> 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.
>

Thanks, this would probably be a better way to approach this.


>
>
>> 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.)
>

I will do that then - revert this commit and split the test patch.
Cheers
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140811/ff13a9f2/attachment.html>