[cmake-developers] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

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.despres at gmail.com>
wrote:

>
>
> On Wed, Mar 9, 2016 at 9:41 PM, Ben Boeckel <ben.boeckel at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20160512/09f95b69/attachment-0001.html>