Merge topic 'misc-refactoring'

788e5c1043 Tests: Add tests for `cmDocumentationFormatter::PrintFormatted()`
8c1a850c19 cmMessenger: Deduplicate `cmSystemTools::Message()` calls
9c118ae9d4 Refactor: `cmCPackGenerator::DisplayVerboseOutput()` unused arg
5fd795f975 MessageType: Add `UNDEFINED` enum item
a6cae9dbc4 cmMessageCommand: Rename+move `CheckingType` → `Message::CheckType`
bb9071e9e7 cmDocumentationFormatter.cxx: Move padding string creation out of loop
f40712fc10 cmTargetLinkLibrariesCommand: Optimize `ostream::operator<<` calls
1ffb746c92 cmGlobalGenerator.cxx: Optimize `ostream::operator<<` calls
...

Acked-by: Kitware Robot <[email protected]>
Merge-request: !9920

Source/CPack/cmCPackGenerator.cxx

@@ -51,10 +51,9 @@ cmCPackGenerator::~cmCPackGenerator()
 }
 
 void cmCPackGenerator::DisplayVerboseOutput(const std::string& msg,
-                                            float progress)
+                                            float /*unused*/)
 {
-  (void)progress;
-  cmCPackLogger(cmCPackLog::LOG_VERBOSE, "" << msg << std::endl);
+  cmCPackLogger(cmCPackLog::LOG_VERBOSE, msg << std::endl);
 }
 
 int cmCPackGenerator::PrepareNames()

Source/CTest/cmCTestBuildAndTest.cxx

@@ -66,15 +66,15 @@ bool cmCTestBuildAndTest::RunCMake(cmake* cm)
   }
   std::cout << "======== CMake output     ======\n";
   if (cm->Run(args) != 0) {
-    std::cout << "======== End CMake output ======\n";
-    std::cout << "Error: cmake execution failed\n";
+    std::cout << "======== End CMake output ======\n"
+                 "Error: cmake execution failed\n";
     return false;
   }
   // do another config?
   if (this->BuildTwoConfig) {
     if (cm->Run(args) != 0) {
-      std::cout << "======== End CMake output ======\n";
-      std::cout << "Error: cmake execution failed\n";
+      std::cout << "======== End CMake output ======\n"
+                   "Error: cmake execution failed\n";
       return false;
     }
   }
@@ -292,9 +292,9 @@ int cmCTestBuildAndTest::Run()
       << "Could not find path to executable, perhaps it was not built: "
       << this->TestCommand << "\n"
       << "tried to find it in these places:\n"
-      << fullPath << "\n";
+      << fullPath << '\n';
     for (std::string const& fail : failed) {
-      std::cout << fail << "\n";
+      std::cout << fail << '\n';
     }
     return 1;
   }
@@ -307,17 +307,17 @@ int cmCTestBuildAndTest::Run()
   int retval = 0;
   // run the test from the this->BuildRunDir if set
   if (!this->BuildRunDir.empty()) {
-    std::cout << "Run test in directory: " << this->BuildRunDir << "\n";
+    std::cout << "Run test in directory: " << this->BuildRunDir << '\n';
     if (!workdir.SetDirectory(this->BuildRunDir)) {
       std::cout << workdir.GetError() << '\n';
       return 1;
     }
   }
-  std::cout << "Running test command: \"" << fullPath << "\"";
+  std::cout << "Running test command: \"" << fullPath << '"';
   for (std::string const& testCommandArg : this->TestCommandArgs) {
-    std::cout << " \"" << testCommandArg << "\"";
+    std::cout << " \"" << testCommandArg << '"';
   }
-  std::cout << "\n";
+  std::cout << '\n';
 
   // how much time is remaining
   cmDuration remainingTime = std::chrono::seconds(0);
@@ -333,7 +333,7 @@ int cmCTestBuildAndTest::Run()
   bool runTestRes = this->RunTest(testCommand, &retval, remainingTime);
 
   if (!runTestRes || retval != 0) {
-    std::cout << "\nTest command failed: " << testCommand[0] << "\n";
+    std::cout << "\nTest command failed: " << testCommand[0] << '\n';
     retval = 1;
   }
 

Source/cmDocumentationFormatter.cxx

@@ -126,6 +126,8 @@ void cmDocumentationFormatter::PrintColumn(std::ostream& os,
   const std::ptrdiff_t width = this->TextWidth - this->TextIndent;
   std::ptrdiff_t column = 0;
 
+  const auto padding = std::string(this->TextIndent, ' ');
+
   // Loop until the end of the text.
   for (const char *l = text.c_str(), *r = skipToSpace(text.c_str()); *l;
        l = skipSpaces(r), r = skipToSpace(l)) {
@@ -141,7 +143,7 @@ void cmDocumentationFormatter::PrintColumn(std::ostream& os,
         } else if (!firstLine && this->TextIndent) {
           // First word on line.  Print indentation unless this is the
           // first line.
-          os << std::string(this->TextIndent, ' ');
+          os << padding;
         }
 
         // Print the word.
@@ -164,7 +166,7 @@ void cmDocumentationFormatter::PrintColumn(std::ostream& os,
       os << '\n';
       firstLine = false;
       if (r > l) {
-        os << std::string(this->TextIndent, ' ');
+        os << padding;
         os.write(l, r - l);
         column = r - l;
         newSentence = (*(r - 1) == '.');

Source/cmGlobalGenerator.cxx

@@ -1497,12 +1497,14 @@ bool cmGlobalGenerator::CheckALLOW_DUPLICATE_CUSTOM_TARGETS() const
 
   // This generator does not support duplicate custom targets.
   std::ostringstream e;
+  // clang-format off
   e << "This project has enabled the ALLOW_DUPLICATE_CUSTOM_TARGETS "
-    << "global property.  "
-    << "The \"" << this->GetName() << "\" generator does not support "
-    << "duplicate custom targets.  "
-    << "Consider using a Makefiles generator or fix the project to not "
-    << "use duplicate target names.";
+       "global property.  "
+       "The \"" << this->GetName() << "\" generator does not support "
+       "duplicate custom targets.  "
+       "Consider using a Makefiles generator or fix the project to not "
+       "use duplicate target names.";
+  // clang-format on
   cmSystemTools::Error(e.str());
   return false;
 }
@@ -1771,11 +1773,12 @@ void cmGlobalGenerator::Generate()
 
   if (!this->CMP0042WarnTargets.empty()) {
     std::ostringstream w;
-    w << cmPolicies::GetPolicyWarning(cmPolicies::CMP0042) << "\n";
-    w << "MACOSX_RPATH is not specified for"
+    w << cmPolicies::GetPolicyWarning(cmPolicies::CMP0042)
+      << "\n"
+         "MACOSX_RPATH is not specified for"
          " the following targets:\n";
     for (std::string const& t : this->CMP0042WarnTargets) {
-      w << " " << t << "\n";
+      w << ' ' << t << '\n';
     }
     this->GetCMakeInstance()->IssueMessage(MessageType::AUTHOR_WARNING,
                                            w.str());
@@ -1792,7 +1795,7 @@ void cmGlobalGenerator::Generate()
       ;
     /* clang-format on */
     for (std::string const& t : this->CMP0068WarnTargets) {
-      w << " " << t << "\n";
+      w << ' ' << t << '\n';
     }
     this->GetCMakeInstance()->IssueMessage(MessageType::AUTHOR_WARNING,
                                            w.str());
@@ -2805,7 +2808,7 @@ static bool RaiseCMP0037Message(cmake* cm, cmTarget* tgt,
   bool issueMessage = false;
   switch (tgt->GetPolicyStatusCMP0037()) {
     case cmPolicies::WARN:
-      e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0037) << "\n";
+      e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0037) << '\n';
       issueMessage = true;
       CM_FALLTHROUGH;
     case cmPolicies::OLD:
@@ -2819,7 +2822,7 @@ static bool RaiseCMP0037Message(cmake* cm, cmTarget* tgt,
   }
   if (issueMessage) {
     e << "The target name \"" << targetNameAsWritten << "\" is reserved "
-      << reason << ".";
+      << reason << '.';
     if (messageType == MessageType::AUTHOR_WARNING) {
       e << "  It may result in undefined behavior.";
     }
@@ -3013,7 +3016,7 @@ void cmGlobalGenerator::ReserveGlobalTargetCodegen()
   bool issueMessage = false;
   switch (policyStatus) {
     case cmPolicies::WARN:
-      e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0171) << "\n";
+      e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0171) << '\n';
       issueMessage = true;
       CM_FALLTHROUGH;
     case cmPolicies::OLD:
@@ -3120,8 +3123,8 @@ void cmGlobalGenerator::AddGlobalTarget_Install(
       std::set<std::string>* componentsSet = &this->InstallComponents;
       std::ostringstream ostr;
       if (!componentsSet->empty()) {
-        ostr << "Available install components are: ";
-        ostr << cmWrap('"', *componentsSet, '"', " ");
+        ostr << "Available install components are: "
+             << cmWrap('"', *componentsSet, '"', " ");
       } else {
         ostr << "Only default component available";
       }
@@ -3736,7 +3739,7 @@ void cmGlobalGenerator::WriteRuleHashes(std::string const& pfile)
     fout << "# Hashes of file build rules.\n";
     for (auto const& rh : this->RuleHashes) {
       fout.write(rh.second.Data, 32);
-      fout << " " << rh.first << "\n";
+      fout << ' ' << rh.first << '\n';
     }
   }
 }
@@ -3754,7 +3757,7 @@ void cmGlobalGenerator::WriteSummary()
         continue;
       }
       this->WriteSummary(tgt.get());
-      fout << tgt->GetSupportDirectory() << "\n";
+      fout << tgt->GetSupportDirectory() << '\n';
     }
   }
 }
@@ -3792,7 +3795,7 @@ void cmGlobalGenerator::WriteSummary(cmGeneratorTarget* target)
       if (!labels.empty()) {
         fout << "# Target labels\n";
         for (std::string const& l : labels) {
-          fout << " " << l << "\n";
+          fout << ' ' << l << '\n';
           lj_target_labels.append(l);
         }
       }
@@ -3815,12 +3818,12 @@ void cmGlobalGenerator::WriteSummary(cmGeneratorTarget* target)
     }
 
     for (auto const& li : directoryLabelsList) {
-      fout << " " << li << "\n";
+      fout << ' ' << li << '\n';
       lj_target_labels.append(li);
     }
 
     for (auto const& li : cmakeDirectoryLabelsList) {
-      fout << " " << li << "\n";
+      fout << ' ' << li << '\n';
       lj_target_labels.append(li);
     }
 
@@ -3837,13 +3840,13 @@ void cmGlobalGenerator::WriteSummary(cmGeneratorTarget* target)
     for (cmSourceFile* sf : cmMakeRange(sources.cbegin(), sourcesEnd)) {
       Json::Value& lj_source = lj_sources.append(Json::objectValue);
       std::string const& sfp = sf->ResolveFullPath();
-      fout << sfp << "\n";
+      fout << sfp << '\n';
       lj_source["file"] = sfp;
       if (cmValue svalue = sf->GetProperty("LABELS")) {
         Json::Value& lj_source_labels = lj_source["labels"] = Json::arrayValue;
         labels.assign(*svalue);
         for (auto const& label : labels) {
-          fout << " " << label << "\n";
+          fout << ' ' << label << '\n';
           lj_source_labels.append(label);
         }
       }

Source/cmMessageCommand.cxx

@@ -26,14 +26,6 @@
 
 namespace {
 
-enum class CheckingType
-{
-  UNDEFINED,
-  CHECK_START,
-  CHECK_PASS,
-  CHECK_FAIL
-};
-
 std::string IndentText(std::string text, cmMakefile& mf)
 {
   auto indent =
@@ -106,7 +98,7 @@ bool cmMessageCommand(std::vector<std::string> const& args,
   auto type = MessageType::MESSAGE;
   auto fatal = false;
   auto level = Message::LogLevel::LOG_UNDEFINED;
-  auto checkingType = CheckingType::UNDEFINED;
+  auto checkingType = Message::CheckType::UNDEFINED;
   if (*i == "SEND_ERROR") {
     type = MessageType::FATAL_ERROR;
     level = Message::LogLevel::LOG_ERROR;
@@ -135,15 +127,15 @@ bool cmMessageCommand(std::vector<std::string> const& args,
     ++i;
   } else if (*i == "CHECK_START") {
     level = Message::LogLevel::LOG_STATUS;
-    checkingType = CheckingType::CHECK_START;
+    checkingType = Message::CheckType::CHECK_START;
     ++i;
   } else if (*i == "CHECK_PASS") {
     level = Message::LogLevel::LOG_STATUS;
-    checkingType = CheckingType::CHECK_PASS;
+    checkingType = Message::CheckType::CHECK_PASS;
     ++i;
   } else if (*i == "CHECK_FAIL") {
     level = Message::LogLevel::LOG_STATUS;
-    checkingType = CheckingType::CHECK_FAIL;
+    checkingType = Message::CheckType::CHECK_FAIL;
     ++i;
   } else if (*i == "CONFIGURE_LOG") {
 #ifndef CMAKE_BOOTSTRAP
@@ -217,16 +209,16 @@ bool cmMessageCommand(std::vector<std::string> const& args,
 
     case Message::LogLevel::LOG_STATUS:
       switch (checkingType) {
-        case CheckingType::CHECK_START:
+        case Message::CheckType::CHECK_START:
           mf.DisplayStatus(IndentText(message, mf), -1);
           mf.GetCMakeInstance()->PushCheckInProgressMessage(message);
           break;
 
-        case CheckingType::CHECK_PASS:
+        case Message::CheckType::CHECK_PASS:
           ReportCheckResult("CHECK_PASS"_s, message, mf);
           break;
 
-        case CheckingType::CHECK_FAIL:
+        case Message::CheckType::CHECK_FAIL:
           ReportCheckResult("CHECK_FAIL"_s, message, mf);
           break;
 

Source/cmMessageType.h

@@ -6,6 +6,7 @@
 
 enum class MessageType
 {
+  UNDEFINED,
   AUTHOR_WARNING,
   AUTHOR_ERROR,
   FATAL_ERROR,
@@ -31,4 +32,13 @@ enum class LogLevel
   LOG_DEBUG,
   LOG_TRACE
 };
+
+enum class CheckType
+{
+  UNDEFINED,
+  CHECK_START,
+  CHECK_PASS,
+  CHECK_FAIL
+};
+
 }

Source/cmMessenger.cxx

@@ -102,11 +102,10 @@ void displayMessage(MessageType t, std::ostringstream& msg)
       t == MessageType::DEPRECATION_ERROR || t == MessageType::AUTHOR_ERROR) {
     cmSystemTools::SetErrorOccurred();
     md.title = "Error";
-    cmSystemTools::Message(msg.str(), md);
   } else {
     md.title = "Warning";
-    cmSystemTools::Message(msg.str(), md);
   }
+  cmSystemTools::Message(msg.str(), md);
 }
 
 void PrintCallStack(std::ostream& out, cmListFileBacktrace bt,

Source/cmTargetLinkLibrariesCommand.cxx

@@ -97,8 +97,8 @@ bool cmTargetLinkLibrariesCommand(std::vector<std::string> const& args,
   if (!target) {
     MessageType t = MessageType::FATAL_ERROR; // fail by default
     std::ostringstream e;
-    e << "Cannot specify link libraries for target \"" << args[0] << "\" "
-      << "which is not built by this project.";
+    e << "Cannot specify link libraries for target \"" << args[0]
+      << "\" which is not built by this project.";
     // The bad target is the only argument. Check how policy CMP0016 is set,
     // and accept, warn or fail respectively:
     if (args.size() < 2) {
@@ -107,9 +107,9 @@ bool cmTargetLinkLibrariesCommand(std::vector<std::string> const& args,
           t = MessageType::AUTHOR_WARNING;
           // Print the warning.
           e << "\n"
-            << "CMake does not support this but it used to work accidentally "
-            << "and is being allowed for compatibility."
-            << "\n"
+               "CMake does not support this but it used to work accidentally "
+               "and is being allowed for compatibility."
+               "\n"
             << cmPolicies::GetPolicyWarning(cmPolicies::CMP0016);
           break;
         case cmPolicies::OLD: // OLD behavior does not warn.
@@ -117,7 +117,7 @@ bool cmTargetLinkLibrariesCommand(std::vector<std::string> const& args,
           break;
         case cmPolicies::REQUIRED_IF_USED:
         case cmPolicies::REQUIRED_ALWAYS:
-          e << "\n" << cmPolicies::GetRequiredPolicyError(cmPolicies::CMP0016);
+          e << '\n' << cmPolicies::GetRequiredPolicyError(cmPolicies::CMP0016);
           break;
         case cmPolicies::NEW: // NEW behavior prints the error.
           break;
@@ -145,7 +145,7 @@ bool cmTargetLinkLibrariesCommand(std::vector<std::string> const& args,
     MessageType messageType = MessageType::AUTHOR_WARNING;
     switch (mf.GetPolicyStatus(cmPolicies::CMP0039)) {
       case cmPolicies::WARN:
-        e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0039) << "\n";
+        e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0039) << '\n';
         modal = "should";
         CM_FALLTHROUGH;
       case cmPolicies::OLD:
@@ -456,7 +456,7 @@ bool TLL::HandleLibrary(ProcessingState currentProcessingState,
     MessageType messageType = MessageType::AUTHOR_WARNING;
     switch (this->Makefile.GetPolicyStatus(cmPolicies::CMP0023)) {
       case cmPolicies::WARN:
-        e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0023) << "\n";
+        e << cmPolicies::GetPolicyWarning(cmPolicies::CMP0023) << '\n';
         modal = "should";
         CM_FALLTHROUGH;
       case cmPolicies::OLD:

Tests/CMakeLib/CMakeLists.txt

@@ -16,6 +16,7 @@ set(CMakeLib_TESTS
   testCTestResourceSpec.cxx
   testCTestResourceGroups.cxx
   testDebug.cxx
+  testDocumentationFormatter.cxx
   testGccDepfileReader.cxx
   testGeneratedFileStream.cxx
   testJSONHelpers.cxx

Tests/CMakeLib/testDocumentationFormatter.cxx

@@ -0,0 +1,138 @@
+/* Distributed under the OSI-approved BSD 3-Clause License.  See accompanying
+   file Copyright.txt or https://cmake.org/licensing for details.  */
+
+#include <sstream>
+#include <string>
+#include <utility>
+
+#include <cmDocumentationFormatter.h>
+
+#include "testCommon.h"
+
+namespace {
+using TestCases =
+  const std::initializer_list<std::pair<std::string, std::string>>;
+
+bool testPrintFormattedNoIndent()
+{
+  const TestCases testCases = {
+    { "", "" },
+    { "One line no EOL text", "One line no EOL text\n" },
+    { "Short text. Two sentences.", "Short text.  Two sentences.\n" },
+    { "Short text\non\nmultiple\nlines\n",
+      "Short text\n\non\n\nmultiple\n\nlines\n\n" },
+    { "Just one a very long word: "
+      "01234567890123456789012345678901234567890123456789012345"
+      "678901234567890123456789",
+      "Just one a very long "
+      "word:\n01234567890123456789012345678901234567890123456789012345"
+      "678901234567890123456789\n" },
+    { " Pre-formatted paragraph with the very long word stays the same: "
+      "0123456789012345678901234567890123456789012345678901234567890123456789",
+      " Pre-formatted paragraph with the very long word stays the same: "
+      "0123456789012345678901234567890123456789012345678901234567890123456789"
+      "\n" },
+    { "Extra  spaces  are     removed.   However, \n   not in    a "
+      "pre-formatted\n  "
+      "paragraph",
+      "Extra spaces are removed.  However,\n\n   not in    a pre-formatted\n  "
+      "paragraph\n" },
+    { "This is the text paragraph longer than a pre-defined wrapping position "
+      "of the `cmDocumentationFormatter` class. And it's gonna be wrapped "
+      "over multiple lines!",
+      "This is the text paragraph longer than a pre-defined wrapping position "
+      "of the\n`cmDocumentationFormatter` class.  And it's gonna be wrapped "
+      "over multiple\nlines!\n" }
+  };
+
+  cmDocumentationFormatter formatter;
+
+  for (auto& test : testCases) {
+    std::ostringstream out;
+    formatter.PrintFormatted(out, test.first);
+    ASSERT_EQUAL(out.str(), test.second);
+  }
+
+  return true;
+}
+
+bool testPrintFormattedIndent2()
+{
+  const TestCases testCases = {
+    { "", "" },
+    { "One line no EOL text", "  One line no EOL text\n" },
+    { "Short text. Two sentences.", "  Short text.  Two sentences.\n" },
+    { "Short text\non\nmultiple\nlines\n",
+      "  Short text\n\n  on\n\n  multiple\n\n  lines\n\n" },
+    { "Just one a very long word: "
+      "01234567890123456789012345678901234567890123456789012345"
+      "678901234567890123456789",
+      "  Just one a very long "
+      "word:\n  01234567890123456789012345678901234567890123456789012345"
+      "678901234567890123456789\n" },
+    { " Pre-formatted paragraph with the very long word stays the same: "
+      "0123456789012345678901234567890123456789012345678901234567890123456789",
+      "   Pre-formatted paragraph with the very long word stays the same: "
+      "0123456789012345678901234567890123456789012345678901234567890123456789"
+      "\n" },
+    { "Extra  spaces  are     removed.   However, \n  not in    a "
+      "pre-formatted\n  "
+      "paragraph",
+      "  Extra spaces are removed.  However,\n\n    not in    a "
+      "pre-formatted\n    "
+      "paragraph\n" },
+    { "This is the text paragraph longer than a pre-defined wrapping position "
+      "of the `cmDocumentationFormatter` class. And it's gonna be wrapped "
+      "over multiple lines!",
+      "  This is the text paragraph longer than a pre-defined wrapping "
+      "position of\n"
+      "  the `cmDocumentationFormatter` class.  And it's gonna be wrapped "
+      "over\n  multiple lines!\n" }
+  };
+
+  cmDocumentationFormatter formatter;
+  formatter.SetIndent(2);
+
+  for (auto& test : testCases) {
+    std::ostringstream out;
+    formatter.PrintFormatted(out, test.first);
+    ASSERT_EQUAL(out.str(), test.second);
+  }
+
+  return true;
+}
+
+bool testPrintFormattedIndent10()
+{
+  const TestCases testCases = {
+    { "", "" },
+    { "One line no EOL text", "          One line no EOL text\n" },
+    { "This is the text paragraph longer than a pre-defined wrapping position "
+      "of the `cmDocumentationFormatter` class. And it's gonna be wrapped "
+      "over multiple lines!",
+      "          This is the text paragraph longer than a pre-defined "
+      "wrapping\n"
+      "          position of the `cmDocumentationFormatter` class.  "
+      "And it's gonna\n"
+      "          be wrapped over multiple lines!\n" }
+  };
+
+  cmDocumentationFormatter formatter;
+  formatter.SetIndent(10);
+
+  for (auto& test : testCases) {
+    std::ostringstream out;
+    formatter.PrintFormatted(out, test.first);
+    ASSERT_EQUAL(out.str(), test.second);
+  }
+
+  return true;
+}
+}
+
+int testDocumentationFormatter(int /*unused*/, char* /*unused*/[])
+{
+  return runTests({ testPrintFormattedNoIndent, testPrintFormattedIndent2,
+                    testPrintFormattedIndent10 },
+                  false);
+}