From f6211f57d6b350dd8d701c19f80a0a458203e909 Mon Sep 17 00:00:00 2001 From: Brad King Date: Wed, 21 Aug 2019 15:03:56 -0400 Subject: cmGlobalVisualStudioGenerator: Fix buffer sizes used RegQueryValueExW In commit 0b9906c2fb (Windows: Use wide-character system APIs, 2013-12-04, v3.0.0-rc1~254^2) several buffer size computations had to be updated to multiply by `sizeof(wchar_t)`, but some for RegQueryValueExW were incorrect because the number of bytes was already computed. Issue: #19610 --- Source/cmGlobalVisualStudioGenerator.cxx | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Source/cmGlobalVisualStudioGenerator.cxx b/Source/cmGlobalVisualStudioGenerator.cxx index 18cf922..fea7bfd 100644 --- a/Source/cmGlobalVisualStudioGenerator.cxx +++ b/Source/cmGlobalVisualStudioGenerator.cxx @@ -572,8 +572,8 @@ bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, if (ERROR_SUCCESS == result) { DWORD valueType = REG_SZ; wchar_t data1[256]; - DWORD cch_data1 = sizeof(data1) * sizeof(data1[0]); - RegQueryValueExW(hsubkey, L"Path", 0, &valueType, (LPBYTE)&data1[0], + DWORD cch_data1 = sizeof(data1); + RegQueryValueExW(hsubkey, L"Path", 0, &valueType, (LPBYTE)data1, &cch_data1); DWORD data2 = 0; @@ -649,9 +649,8 @@ bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, if (ERROR_SUCCESS == result) { DWORD valueType = REG_SZ; wchar_t data1[256]; - DWORD cch_data1 = sizeof(data1) * sizeof(data1[0]); - RegQueryValueExW(hkey, L"Path", 0, &valueType, (LPBYTE)&data1[0], - &cch_data1); + DWORD cch_data1 = sizeof(data1); + RegQueryValueExW(hkey, L"Path", 0, &valueType, (LPBYTE)data1, &cch_data1); DWORD data2 = 0; DWORD cch_data2 = sizeof(data2); -- cgit v0.12 From a7aade84198343af9de79fda5c37560a2f9531a4 Mon Sep 17 00:00:00 2001 From: Brad King Date: Wed, 21 Aug 2019 15:03:56 -0400 Subject: cmGlobalVisualStudioGenerator: Fix buffer sizes used with RegEnumKeyExW In commit 0b9906c2fb (Windows: Use wide-character system APIs, 2013-12-04, v3.0.0-rc1~254^2) several buffer size computations had to be updated to multiply by `sizeof(wchar_t)`, but for RegEnumKeyExW we were already computing the correct number of characters with a division which was accidentally converted to a multiplication. Use `cm::size` to compute the number of characters in the buffer instead. Issue: #19610 --- Source/cmGlobalVisualStudioGenerator.cxx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Source/cmGlobalVisualStudioGenerator.cxx b/Source/cmGlobalVisualStudioGenerator.cxx index fea7bfd..724210e 100644 --- a/Source/cmGlobalVisualStudioGenerator.cxx +++ b/Source/cmGlobalVisualStudioGenerator.cxx @@ -556,9 +556,9 @@ bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, if (ERROR_SUCCESS == result) { // Iterate the subkeys and look for the values of interest in each subkey: wchar_t subkeyname[256]; - DWORD cch_subkeyname = sizeof(subkeyname) * sizeof(subkeyname[0]); + DWORD cch_subkeyname = cm::size(subkeyname); wchar_t keyclass[256]; - DWORD cch_keyclass = sizeof(keyclass) * sizeof(keyclass[0]); + DWORD cch_keyclass = cm::size(keyclass); FILETIME lastWriteTime; lastWriteTime.dwHighDateTime = 0; lastWriteTime.dwLowDateTime = 0; @@ -621,8 +621,8 @@ bool IsVisualStudioMacrosFileRegistered(const std::string& macrosFile, } ++index; - cch_subkeyname = sizeof(subkeyname) * sizeof(subkeyname[0]); - cch_keyclass = sizeof(keyclass) * sizeof(keyclass[0]); + cch_subkeyname = cm::size(subkeyname); + cch_keyclass = cm::size(keyclass); lastWriteTime.dwHighDateTime = 0; lastWriteTime.dwLowDateTime = 0; } -- cgit v0.12 From a8ca5aea946e1154b4518ba28db3f4e695dbf165 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 10:51:12 -0400 Subject: cmMakefileTargetGenerator: Check for null before using a pointer Fix the order of logic added by commit 7740ccd1a4 (ENH: some cleanup of the makefile generator, 2006-02-14, v2.4.0~517) to check for allocation failure ('new' returns null) before using the pointer. Issue: #19610 --- Source/cmMakefileTargetGenerator.cxx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Source/cmMakefileTargetGenerator.cxx b/Source/cmMakefileTargetGenerator.cxx index f35df32..0f13540 100644 --- a/Source/cmMakefileTargetGenerator.cxx +++ b/Source/cmMakefileTargetGenerator.cxx @@ -131,10 +131,10 @@ void cmMakefileTargetGenerator::CreateRuleFile() this->BuildFileStream = new cmGeneratedFileStream(this->BuildFileNameFull, false, this->GlobalGenerator->GetMakefileEncoding()); - this->BuildFileStream->SetCopyIfDifferent(true); if (!this->BuildFileStream) { return; } + this->BuildFileStream->SetCopyIfDifferent(true); this->LocalGenerator->WriteDisclaimer(*this->BuildFileStream); if (this->GlobalGenerator->AllowDeleteOnError()) { std::vector no_depends; @@ -300,10 +300,10 @@ void cmMakefileTargetGenerator::WriteCommonCodeRules() this->FlagFileStream = new cmGeneratedFileStream(this->FlagFileNameFull, false, this->GlobalGenerator->GetMakefileEncoding()); - this->FlagFileStream->SetCopyIfDifferent(true); if (!this->FlagFileStream) { return; } + this->FlagFileStream->SetCopyIfDifferent(true); this->LocalGenerator->WriteDisclaimer(*this->FlagFileStream); // Include the flags for the target. @@ -1033,10 +1033,10 @@ void cmMakefileTargetGenerator::WriteTargetDependRules() this->InfoFileNameFull = this->LocalGenerator->ConvertToFullPath(this->InfoFileNameFull); this->InfoFileStream = new cmGeneratedFileStream(this->InfoFileNameFull); - this->InfoFileStream->SetCopyIfDifferent(true); - if (!*this->InfoFileStream) { + if (!this->InfoFileStream) { return; } + this->InfoFileStream->SetCopyIfDifferent(true); this->LocalGenerator->WriteDependLanguageInfo(*this->InfoFileStream, this->GeneratorTarget); -- cgit v0.12 From 3f4c4e7afe6075b18f99c67cf808e2300b3690c4 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 10:56:55 -0400 Subject: cmVSSetupHelper: Fix SmartBSTR copy operations Fix the SmartBSTR copy constructor and copy assignment operator added by commit 18c8278b62 (VS: Add helper class to interact with Visual Studio Installer, 2016-12-14, v3.8.0-rc1~93^2~4) to use the string from the source of the copy. Issue: #19610 --- Source/cmVSSetupHelper.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Source/cmVSSetupHelper.h b/Source/cmVSSetupHelper.h index 1bda54a..9382c05 100644 --- a/Source/cmVSSetupHelper.h +++ b/Source/cmVSSetupHelper.h @@ -77,7 +77,8 @@ public: SmartBSTR(const SmartBSTR& src) { if (src.str != NULL) { - str = ::SysAllocStringByteLen((char*)str, ::SysStringByteLen(str)); + str = + ::SysAllocStringByteLen((char*)src.str, ::SysStringByteLen(src.str)); } else { str = ::SysAllocStringByteLen(NULL, 0); } @@ -87,7 +88,8 @@ public: if (str != src.str) { ::SysFreeString(str); if (src.str != NULL) { - str = ::SysAllocStringByteLen((char*)str, ::SysStringByteLen(str)); + str = + ::SysAllocStringByteLen((char*)src.str, ::SysStringByteLen(src.str)); } else { str = ::SysAllocStringByteLen(NULL, 0); } -- cgit v0.12 From b1cfaf7b91f87fed0c70a6a7763d565023420788 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 11:21:04 -0400 Subject: cmVSSetupHelper: Remove unused SmartBSTR copy operations For our use case we do not actually need to copy these. Mark the operations as `= delete` to simplify the code. --- Source/cmVSSetupHelper.h | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/Source/cmVSSetupHelper.h b/Source/cmVSSetupHelper.h index 9382c05..ad46c75 100644 --- a/Source/cmVSSetupHelper.h +++ b/Source/cmVSSetupHelper.h @@ -74,28 +74,8 @@ class SmartBSTR { public: SmartBSTR() { str = NULL; } - SmartBSTR(const SmartBSTR& src) - { - if (src.str != NULL) { - str = - ::SysAllocStringByteLen((char*)src.str, ::SysStringByteLen(src.str)); - } else { - str = ::SysAllocStringByteLen(NULL, 0); - } - } - SmartBSTR& operator=(const SmartBSTR& src) - { - if (str != src.str) { - ::SysFreeString(str); - if (src.str != NULL) { - str = - ::SysAllocStringByteLen((char*)src.str, ::SysStringByteLen(src.str)); - } else { - str = ::SysAllocStringByteLen(NULL, 0); - } - } - return *this; - } + SmartBSTR(const SmartBSTR& src) = delete; + SmartBSTR& operator=(const SmartBSTR& src) = delete; operator BSTR() const { return str; } BSTR* operator&() throw() { return &str; } ~SmartBSTR() throw() { ::SysFreeString(str); } -- cgit v0.12 From 51565abe7929f4baf5772345dc8d9c62ece9cf7a Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 11:26:35 -0400 Subject: cmMessageCommand: Remove extra layer of parentheses The logic checking `CMAKE_WARN_DEPRECATED` contained an unnecessary layer of parentheses. The condition is of the form `!IsSet || IsOn` which is correct because the documentation says that the behavior is enabled unless the variable is explicitly set to a false value. Issue: #19610 --- Source/cmMessageCommand.cxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/cmMessageCommand.cxx b/Source/cmMessageCommand.cxx index 3f33312..39b40b8 100644 --- a/Source/cmMessageCommand.cxx +++ b/Source/cmMessageCommand.cxx @@ -70,8 +70,8 @@ bool cmMessageCommand(std::vector const& args, fatal = true; type = MessageType::DEPRECATION_ERROR; level = cmake::LogLevel::LOG_ERROR; - } else if ((!status.GetMakefile().IsSet("CMAKE_WARN_DEPRECATED") || - status.GetMakefile().IsOn("CMAKE_WARN_DEPRECATED"))) { + } else if (!status.GetMakefile().IsSet("CMAKE_WARN_DEPRECATED") || + status.GetMakefile().IsOn("CMAKE_WARN_DEPRECATED")) { type = MessageType::DEPRECATION_WARNING; level = cmake::LogLevel::LOG_WARNING; } else { -- cgit v0.12 From 303e81343804d5ef271a3669c1e6cac558055ca1 Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 11:32:54 -0400 Subject: CTest: Simplify some boolean conditions Directly compare two boolean values instead of spelling out accepted combinations. Issue: #19610 --- Source/CTest/cmCTestRunTest.cxx | 3 +-- Source/CTest/cmCTestTestHandler.cxx | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx index fb91322..d232c23 100644 --- a/Source/CTest/cmCTestRunTest.cxx +++ b/Source/CTest/cmCTestRunTest.cxx @@ -148,8 +148,7 @@ bool cmCTestRunTest::EndTest(size_t completed, size_t total, bool started) this->TestResult.CompletionStatus = s.str(); cmCTestLog(this->CTest, HANDLER_OUTPUT, "***Skipped "); skipped = true; - } else if ((success && !this->TestProperties->WillFail) || - (!success && this->TestProperties->WillFail)) { + } else if (success != this->TestProperties->WillFail) { this->TestResult.Status = cmCTestTestHandler::COMPLETED; outputStream << " Passed "; } else { diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx index 7ae0d26..8430ef4 100644 --- a/Source/CTest/cmCTestTestHandler.cxx +++ b/Source/CTest/cmCTestTestHandler.cxx @@ -700,8 +700,7 @@ void cmCTestTestHandler::PrintLabelOrSubprojectSummary(bool doSubProject) } // if we are doing sub projects and this label is one, then use it // if we are not doing sub projects and the label is not one use it - if ((doSubProject && isSubprojectLabel) || - (!doSubProject && !isSubprojectLabel)) { + if (doSubProject == isSubprojectLabel) { if (l.size() > maxlen) { maxlen = l.size(); } -- cgit v0.12 From 7c2767ef3b575bc4b27d4d57ab9f2d5d1acbd3ce Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 11:52:41 -0400 Subject: cmCTestMultiProcessHandler: Explain testRun ownership in comments The ownership semantics of the 'testRun' variable are subtle and may fool static analysers. Add comments explaining them for now. Later some refactoring could be done to clarify the code. Issue: #19610 --- Source/CTest/cmCTestMultiProcessHandler.cxx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx index 2d07420..21d5595 100644 --- a/Source/CTest/cmCTestMultiProcessHandler.cxx +++ b/Source/CTest/cmCTestMultiProcessHandler.cxx @@ -189,10 +189,13 @@ bool cmCTestMultiProcessHandler::StartTestProcess(int test) std::strerror(workdir.GetLastResult())); } else { if (testRun->StartTest(this->Completed, this->Total)) { + // Ownership of 'testRun' has moved to another structure. + // When the test finishes, FinishTestProcess will be called. return true; } } + // Pass ownership of 'testRun'. this->FinishTestProcess(testRun, false); return false; } -- cgit v0.12 From 74f2c0ea56fa3b9dce539fe8cbe0f0f7690b658d Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 11:05:55 -0400 Subject: cmCTestTestHandler: Remove extra layer of parentheses A condition in `ComputeTestListForRerunFailed` contained an extra layer of parentheses. Remove them. The condition itself is correct because an empty list means "all tests" so we want to include the current test. Issue: #19610 --- Source/CTest/cmCTestTestHandler.cxx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx index 8430ef4..db214e1 100644 --- a/Source/CTest/cmCTestTestHandler.cxx +++ b/Source/CTest/cmCTestTestHandler.cxx @@ -893,7 +893,7 @@ void cmCTestTestHandler::ComputeTestListForRerunFailed() cnt++; // if this test is not in our list of tests to run, then skip it. - if ((!this->TestsToRun.empty() && !cmContains(TestsToRun, cnt))) { + if (!this->TestsToRun.empty() && !cmContains(this->TestsToRun, cnt)) { continue; } -- cgit v0.12 From 7fe3e874d59fb054a092738884157153c5550efc Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 22 Aug 2019 11:36:49 -0400 Subject: cmCPackLog: Fix support for multiple log message tags Fix logic from commit bbf1c2d275 (ENH: More improvements and add logging, 2006-01-02, v2.4.0~712) to append to the accumulated tag string instead of overwriting it for each type of message. Issue: #19610 --- Source/CPack/cmCPackLog.cxx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Source/CPack/cmCPackLog.cxx b/Source/CPack/cmCPackLog.cxx index a3ca4b5..ca675fd 100644 --- a/Source/CPack/cmCPackLog.cxx +++ b/Source/CPack/cmCPackLog.cxx @@ -83,7 +83,7 @@ void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, if (!tagString.empty()) { tagString += ","; } - tagString = "VERBOSE"; + tagString += "VERBOSE"; } } if (tag & LOG_WARNING) { @@ -93,7 +93,7 @@ void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, if (!tagString.empty()) { tagString += ","; } - tagString = "WARNING"; + tagString += "WARNING"; } } if (tag & LOG_ERROR) { @@ -103,7 +103,7 @@ void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, if (!tagString.empty()) { tagString += ","; } - tagString = "ERROR"; + tagString += "ERROR"; } } if (tag & LOG_DEBUG && this->Debug) { @@ -113,7 +113,7 @@ void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, if (!tagString.empty()) { tagString += ","; } - tagString = "DEBUG"; + tagString += "DEBUG"; } useFileAndLine = true; } @@ -124,7 +124,7 @@ void cmCPackLog::Log(int tag, const char* file, int line, const char* msg, if (!tagString.empty()) { tagString += ","; } - tagString = "VERBOSE"; + tagString += "VERBOSE"; } } if (this->Quiet) { -- cgit v0.12