On Tue, Mar 08, 2016 at 12:36:31 +0100, Nicolas Desprès wrote: > Did you have a chance to review my patches?
So I looked at it today, and it looks good overall. A few niggles: +inline bool cmEndsWith(const std::string& str, const std::string& what) +{ + assert(str.size() >= what.size()); Probably better to just "return false;" if this would fire. Better than crashing if something in a release would have hit this. + return str.compare(str.size() - what.size(), what.size(), what) == 0; +} + +inline void cmStripSuffixIfExists(std::string* str, + const std::string& suffix) +{ + assert(str != NULL); Why not just use a reference rather than a pointer? + if (cmEndsWith(*str, suffix)) + str->resize(str->size() - suffix.size()); Missing braces. -std::string cmGlobalNinjaGenerator::ConvertToNinjaPath(const std::string& path) +static void EnsureTrailingSlash(std::string* path) +{ + assert(path != NULL); Same thing: why not a reference? + if (path->empty()) + return; + std::string::value_type last = (*path)[path->size()-1]; +#ifdef _WIN32 + if (last != '\\') + *path += '\\'; +#else + if (last != '/') + *path += '/'; +#endif Missing braces in the if statements here. - cmGlobalNinjaGenerator::WriteDefault(os, - outputs, - "Make the all target the default."); + if (!this->HasOutputPathPrefix()) + cmGlobalNinjaGenerator::WriteDefault(os, + outputs, + "Make the all target the default."); Missing braces. +void +cmGlobalNinjaGenerator::StripNinjaOutputPathPrefixAsSuffix(std::string* path) +{ + assert(path != NULL); More pointers :) . + if (path->empty()) + return; And braces. As for the tests: + file(WRITE "${top_build_ninja}" "\ +subninja sub/build.ninja +default sub/all +") I think adding the (documented) bit: +rule RERUN + command = true + description = Testing regeneration + generator = 1 + +build build.ninja: RERUN || sub/build.ninja + pool = console here and testing that if the CMakeLists.txt is touched that CMake reruns would be worth it. It seems to work here, so keeping it working would be great. Thanks! --Ben -- 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