Tobias Hunger wrote: > Did anyone find some time for a review yet?
Hi Tobias, I had a look through this this evening. Thanks for working on this. The commit adding the functionality at the end looks much better after the extra generator refactoring. Here are some review notes: * That commit has a cmDefinitions include though that should be removed. * There are also some methods that should be const * and a whitespace change that should be squashed into the commit that introduces it * The pretty-print flag should be removed. Aside from being a boolean trap, it creates a interface segregation principle violation. See eg https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=23f87e81 for more. If you want to pretty print things on the command line I suggest cmake -E capabilities | jq which will give you colorized output, or cmake -E capabilities | python -mjson.tool for something that you already have installed. See http://stackoverflow.com/questions/352098/how-can-i-pretty-print-json for more. * The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the capabilities handling: #if defined(CMAKE_BUILD_WITH_CMAKE) else if (args[1] == "capabilities") { cmake cm; std::cout << cm.ReportCapabilities(); return 0; } #endif That define should not be used in ReportCapabilities. * Splitting the 'CMake: Refactor ExtraGenerator registration' commit into multiple commits would make it more reviewable, and more bisectable in the future. As it is, it is doing many different things, none of which are mentioned in the commit message, and some of which it probably shouldn't be doing. For example renaming GetExtraGeneratorName to GetExternalMakefileProjectGeneratorName is probably not needed. If you really want to do it, then it should be in its own commit with its own commit message which justifies the change. As it is, it adds noise to the big commit and makes it harder to review. Minimal is always better with commits which do refactoring like that. A general good rule of thumb (which helps reviews, and makes things bisectable in the future) is to do one thing per commit. Thanks, Steve. -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
