Hi Ben, Brad has just sent a notification email about an upcoming feature freeze. Do you think we could have this feature merged before?
Regards, Nicolas On Sun, Mar 20, 2016 at 1:47 PM, Nicolas Desprès <nicolas.desp...@gmail.com> wrote: > > > On Wed, Mar 9, 2016 at 9:41 PM, Ben Boeckel <ben.boec...@kitware.com> > wrote: > >> 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: >> > Thanks > >> >> +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. >> >> > Ok. > > >> + 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? >> > > I don't like to use non-const reference argument because the caller may > not expect its variable to be modified since it not passed it by address. > Anyway, reference argument are used in other places in the source code so > it does not matter. > > >> >> + if (cmEndsWith(*str, suffix)) >> + str->resize(str->size() - suffix.size()); >> >> Missing braces. >> > > Ok. > > >> >> -std::string cmGlobalNinjaGenerator::ConvertToNinjaPath(const >> std::string& path) >> +static void EnsureTrailingSlash(std::string* path) >> +{ >> + assert(path != NULL); >> >> Same thing: why not a reference? >> > Done. > >> >> + 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. >> >> > Ok. > > >> - 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. >> > Ok. > >> >> +void >> +cmGlobalNinjaGenerator::StripNinjaOutputPathPrefixAsSuffix(std::string* >> path) >> +{ >> + assert(path != NULL); >> >> More pointers :) . >> > Ok. > >> >> + if (path->empty()) >> + return; >> >> And braces. >> > Ok. > > The fixes are that commit: > > https://github.com/nicolasdespres/CMake/commit/3fa749a19847898fcbb5635a273b0d02707dd4bd > > >> 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. >> > > I guess that test should exist on the super-generator side. But it is > probably safer to test it here too. > > The fix is in that commit: > > https://github.com/nicolasdespres/CMake/commit/13f341588bc6ee1cb0ec5dce8f44602f5d066ca9 > > Tell me if you prefer I squash all the commits together before you review. > > Thanks, > > -- > Nicolas Desprès > -- Nicolas Desprès
-- 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