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

[Ben Boeckel]

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