Hi Stephen, thanks for taking the time to do such a thorough review!
I pushed an update that takes most of the feedback into account. Still at: https://github.com/hunger/CMake/commits/cmake-capabilities On Wed, Jul 27, 2016 at 1:11 AM, Stephen Kelly <steve...@gmail.com> wrote: > 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. Gone. > * There are also some methods that should be const Added const to some of them:-) Hope I caught all. > * and a whitespace change that should be squashed into the commit that > introduces it I used Utilities/Scripts/clang-format.bash to do the formatting, so that should not be an issue. I just reran that on all commits. Maybe I forgot it in a commit or something before. > * The pretty-print flag should be removed. Aside from being a boolean trap, > it creates a interface segregation principle violation. See eg Removed, even though I find it really useful when testing cmake on mac/windows. I do have all the small helper tools on Linux, but usually not on windows/Mac. > * 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 Why? > That define should not be used in ReportCapabilities. That define seems necessary to keep the bootstrap test running. > * Splitting the 'CMake: Refactor ExtraGenerator registration' commit into > multiple commits would make it more reviewable, and more bisectable in the > future. Sorry, this is what you will get from me: I have to start at some place and meddle through till things work again. I have to back-track often. Basically I need to get some functionality implemented completely and then test that. Only afterwards can I spend time on making the patches pretty. If that is not acceptable, then please feel free to do necessary changes yourself. > 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. I undid that change. That is one of the things that I originally removed and then realized last minute that it is needed somehow. So I added it, not realizing that I had removed similar functionality earlier. > A general good rule of thumb (which helps reviews, and makes things > bisectable in the future) is to do one thing per commit. I agree that this is the ideal we all should all strive for, but you are not going to get that from me anytime soon. At least not in the cmake codebase. Best Regards, Tobias -- 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
