On Mon, Aug 5, 2013 at 6:06 PM, Brad King <[email protected]> wrote:
> On 08/05/2013 11:28 AM, Nicolas Desprès wrote: > > I have just rebased my branch on top of the master and the test suite > still pass. > [snip] > > I really want to have this patch in the next release > > I can't promise to accept it since 2.8.12 rc1 is coming out > tomorrow or Wednesday and this topic isn't even in 'next' yet. > This touches some pretty fundamental logic and could easily > have a subtle behavior change so I consider it high risk. > I understand. I have just found an issue that is not covered by CMake's test suite. It happens when one of the file path is empty. I will add a test for this case tomorrow. > > On 07/29/2013 08:25 AM, Stephen Kelly wrote: > > I think Brad will have to do the rest. > > Sorry, I stopped reading this thread assuming that it would end > with Steve bringing the change to the topic stage at which point > my attention would be drawn again. > No problem. > > I just fetched the large-deps-perf topic from your Github repo. > I've never been happy with the linear search so thanks for working > on this. Here are a couple comments: > > * Please replace large_list.cmake in the test with use of the > foreach() command's RANGE signature and list(APPEND). > Done. > > * Why is special logic in Tests/CMakeLists.txt needed to add > the test case? > Since it is a performance test on configuration phase only, I need a specific timeout value and to run cmake only (neither make, nor any extra program built the test project). I have added a comment saying so. > > * Please add comments explaining in prose what the lookup map > comparison functor is doing and why. I could infer it only > by looking at the old code (which should have been commented > before too). The assert examples are good for verification > but not for understanding from scratch. > I added the comment. I hope it is understandable. The best would be to have a unit test for this method but I can't think of a simple way to do it since there will be too many objects to mock. I have pushed a bunch of fixup commit if you want to review them. I will squash them tomorrow. Thanks, -- Nicolas Desprès
-- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
