Tobias Hunger wrote: > Hi Stephen, > > thanks for taking the time to do such a thorough review!
Actually my review wasn't thorough at all. I didn't try to review it further. The NewFactory methods in your patch don't return a new'd object, but instead return static locals. The regular generators NewFactory methods don't work that way, so you're introducing a pattern which is different to what already exists and the commit message doesn't say why. Maybe there's a reason, but I don't know what it is. In the future, no one else will know either. There are lots of things in there for which your intent is unclear. That's why splitting and writing descriptive commit messages is valuable. You might find https://vimeo.com/172882423 to be a good introduction to this. I have more to ask about your first commit (and why the second commit is split out). I stopped my review at recommending it be split to see what it is hiding. > Added const to some of them:-) Hope I caught all. cmake::ReportCapabilities() should be const, right? >> * 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. Maybe. Running the script on all commits would be the fix anyway. >> * 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? Because it's a bit odd to return 0 if capabilities can't be reported. I missed that the contents of that method don't compile in bootstrap mode though without the define. >> 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. This doesn't happen if you split commits. Splitting makes these things visible to you and you can decide whether they were intentional or not at that point. >> 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. Calling it an 'ideal' isn't really the right mindset. It's easy, and it's generally done by all other cmake contributors. > At least not in the cmake codebase. I don't know what this part means. It seems somehow disdainful, but I might be missing something. Something to talk about in person perhaps. 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