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
--
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
