r215365 - Coverage mapping: emit mapping for cxx constructors that use microsoft's ABI
arphaman at gmail.com
Mon Aug 11 11:23:38 PDT 2014
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
>> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the cfe-commits