From f2a33107be2d7d701708506c4c3054799d941bd6 Mon Sep 17 00:00:00 2001 From: Ben Boeckel Date: Wed, 8 Apr 2020 11:51:36 -0400 Subject: clang-tidy: address bugprone-branch-clone lints Arguably, many of these are bugs in `clang-tidy`. An if/else tree with other conditionals between cloned blocks may be relying on the intermediate logic to fall out of the case and inverting this logic may be non-trivial. See: https://bugs.llvm.org/show_bug.cgi?id=44165 --- Source/CPack/IFW/cmCPackIFWRepository.cxx | 4 ---- Source/CTest/cmCTestCVS.cxx | 2 ++ Source/CTest/cmCTestMemCheckHandler.cxx | 40 ++++++++++--------------------- Source/CTest/cmCTestTestHandler.cxx | 7 +++--- Source/cmCTest.cxx | 12 ++++------ Source/cmComputeLinkInformation.cxx | 1 - Source/cmDependsFortran.cxx | 4 +--- Source/cmExtraCodeLiteGenerator.cxx | 4 +--- Source/cmExtraEclipseCDT4Generator.cxx | 1 - Source/cmExtraKateGenerator.cxx | 5 ++-- Source/cmFindPackageCommand.cxx | 6 +++++ Source/cmGeneratorTarget.cxx | 8 ++++--- Source/cmInstallCommand.cxx | 1 - Source/cmLocalGenerator.cxx | 10 ++++---- Source/cmMakefile.cxx | 5 +--- Source/cmPolicies.cxx | 2 ++ Source/cmRST.cxx | 2 ++ Source/cmSeparateArgumentsCommand.cxx | 2 ++ Source/cmake.cxx | 25 +++++++------------ Source/cmcmd.cxx | 5 ++-- 20 files changed, 59 insertions(+), 87 deletions(-) diff --git a/Source/CPack/IFW/cmCPackIFWRepository.cxx b/Source/CPack/IFW/cmCPackIFWRepository.cxx index a696549..f5e8744 100644 --- a/Source/CPack/IFW/cmCPackIFWRepository.cxx +++ b/Source/CPack/IFW/cmCPackIFWRepository.cxx @@ -21,11 +21,7 @@ bool cmCPackIFWRepository::IsValid() const switch (this->Update) { case cmCPackIFWRepository::None: - valid = !this->Url.empty(); - break; case cmCPackIFWRepository::Add: - valid = !this->Url.empty(); - break; case cmCPackIFWRepository::Remove: valid = !this->Url.empty(); break; diff --git a/Source/CTest/cmCTestCVS.cxx b/Source/CTest/cmCTestCVS.cxx index 45ec390..75b534b 100644 --- a/Source/CTest/cmCTestCVS.cxx +++ b/Source/CTest/cmCTestCVS.cxx @@ -152,6 +152,8 @@ private: this->FinishRevision(); } } else if (this->Section == SectionRevisions) { + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) if (!this->Rev.Log.empty()) { // Continue the existing log. this->Rev.Log += this->Line; diff --git a/Source/CTest/cmCTestMemCheckHandler.cxx b/Source/CTest/cmCTestMemCheckHandler.cxx index c1ecaf1..7ad87f5 100644 --- a/Source/CTest/cmCTestMemCheckHandler.cxx +++ b/Source/CTest/cmCTestMemCheckHandler.cxx @@ -957,35 +957,25 @@ bool cmCTestMemCheckHandler::ProcessMemCheckValgrindOutput( "valgrind line " << lines[cc] << std::endl, this->Quiet); int failure = cmCTestMemCheckHandler::NO_MEMORY_FAULT; - if (vgFIM.find(lines[cc])) { + auto& line = lines[cc]; + if (vgFIM.find(line)) { failure = cmCTestMemCheckHandler::FIM; - } else if (vgFMM.find(lines[cc])) { + } else if (vgFMM.find(line)) { failure = cmCTestMemCheckHandler::FMM; - } else if (vgMLK1.find(lines[cc])) { + } else if (vgMLK1.find(line) || vgMLK2.find(line)) { failure = cmCTestMemCheckHandler::MLK; - } else if (vgMLK2.find(lines[cc])) { - failure = cmCTestMemCheckHandler::MLK; - } else if (vgPAR.find(lines[cc])) { + } else if (vgPAR.find(line)) { failure = cmCTestMemCheckHandler::PAR; - } else if (vgMPK1.find(lines[cc])) { - failure = cmCTestMemCheckHandler::MPK; - } else if (vgMPK2.find(lines[cc])) { + } else if (vgMPK1.find(line) || vgMPK2.find(line)) { failure = cmCTestMemCheckHandler::MPK; - } else if (vgUMC.find(lines[cc])) { + } else if (vgUMC.find(line)) { failure = cmCTestMemCheckHandler::UMC; - } else if (vgUMR1.find(lines[cc])) { - failure = cmCTestMemCheckHandler::UMR; - } else if (vgUMR2.find(lines[cc])) { - failure = cmCTestMemCheckHandler::UMR; - } else if (vgUMR3.find(lines[cc])) { + } else if (vgUMR1.find(line) || vgUMR2.find(line) || vgUMR3.find(line) || + vgUMR4.find(line) || vgUMR5.find(line)) { failure = cmCTestMemCheckHandler::UMR; - } else if (vgUMR4.find(lines[cc])) { - failure = cmCTestMemCheckHandler::UMR; - } else if (vgUMR5.find(lines[cc])) { - failure = cmCTestMemCheckHandler::UMR; - } else if (vgIPW.find(lines[cc])) { + } else if (vgIPW.find(line)) { failure = cmCTestMemCheckHandler::IPW; - } else if (vgABR.find(lines[cc])) { + } else if (vgABR.find(line)) { failure = cmCTestMemCheckHandler::ABR; } @@ -1049,13 +1039,9 @@ bool cmCTestMemCheckHandler::ProcessMemCheckDrMemoryOutput( ostr << l << std::endl; if (drMemoryError.find(l)) { defects++; - if (unaddressableAccess.find(l)) { + if (unaddressableAccess.find(l) || uninitializedRead.find(l)) { results[cmCTestMemCheckHandler::UMR]++; - } else if (uninitializedRead.find(l)) { - results[cmCTestMemCheckHandler::UMR]++; - } else if (leak.find(l)) { - results[cmCTestMemCheckHandler::MLK]++; - } else if (handleLeak.find(l)) { + } else if (leak.find(l) || handleLeak.find(l)) { results[cmCTestMemCheckHandler::MLK]++; } else if (invalidHeapArgument.find(l)) { results[cmCTestMemCheckHandler::FMM]++; diff --git a/Source/CTest/cmCTestTestHandler.cxx b/Source/CTest/cmCTestTestHandler.cxx index f9850ef..ca65946 100644 --- a/Source/CTest/cmCTestTestHandler.cxx +++ b/Source/CTest/cmCTestTestHandler.cxx @@ -2402,10 +2402,9 @@ bool cmCTestTestHandler::AddTest(const std::vector& args) test.SkipReturnCode = -1; test.PreviousRuns = 0; if (this->UseIncludeRegExpFlag && - !this->IncludeTestsRegularExpression.find(testname)) { - test.IsInBasedOnREOptions = false; - } else if (this->UseExcludeRegExpFlag && !this->UseExcludeRegExpFirst && - this->ExcludeTestsRegularExpression.find(testname)) { + (!this->IncludeTestsRegularExpression.find(testname) || + (!this->UseExcludeRegExpFirst && + this->ExcludeTestsRegularExpression.find(testname)))) { test.IsInBasedOnREOptions = false; } this->TestList.push_back(test); diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index c5505f9..35a8952 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -1941,13 +1941,11 @@ bool cmCTest::HandleCommandLineArguments(size_t& i, else if (this->CheckArgument(arg, "--debug"_s)) { this->Impl->Debug = true; this->Impl->ShowLineNumbers = true; - } else if (this->CheckArgument(arg, "--group"_s) && i < args.size() - 1) { - i++; - this->Impl->SpecificGroup = args[i]; - } - // This is an undocumented / deprecated option. - // "Track" has been renamed to "Group". - else if (this->CheckArgument(arg, "--track"_s) && i < args.size() - 1) { + } else if ((this->CheckArgument(arg, "--group"_s) || + // This is an undocumented / deprecated option. + // "Track" has been renamed to "Group". + this->CheckArgument(arg, "--track"_s)) && + i < args.size() - 1) { i++; this->Impl->SpecificGroup = args[i]; } else if (this->CheckArgument(arg, "--show-line-numbers"_s)) { diff --git a/Source/cmComputeLinkInformation.cxx b/Source/cmComputeLinkInformation.cxx index 8d27699..de81f13 100644 --- a/Source/cmComputeLinkInformation.cxx +++ b/Source/cmComputeLinkInformation.cxx @@ -1440,7 +1440,6 @@ void cmComputeLinkInformation::HandleBadFullItem(std::string const& item, } case cmPolicies::OLD: // OLD behavior does not warn. - break; case cmPolicies::NEW: // NEW behavior will not get here. break; diff --git a/Source/cmDependsFortran.cxx b/Source/cmDependsFortran.cxx index 666306c..c45cd1c 100644 --- a/Source/cmDependsFortran.cxx +++ b/Source/cmDependsFortran.cxx @@ -29,12 +29,10 @@ static void cmFortranModuleAppendUpperLower(std::string const& mod, std::string& mod_lower) { std::string::size_type ext_len = 0; - if (cmHasLiteralSuffix(mod, ".mod")) { + if (cmHasLiteralSuffix(mod, ".mod") || cmHasLiteralSuffix(mod, ".sub")) { ext_len = 4; } else if (cmHasLiteralSuffix(mod, ".smod")) { ext_len = 5; - } else if (cmHasLiteralSuffix(mod, ".sub")) { - ext_len = 4; } std::string const& name = mod.substr(0, mod.size() - ext_len); std::string const& ext = mod.substr(mod.size() - ext_len); diff --git a/Source/cmExtraCodeLiteGenerator.cxx b/Source/cmExtraCodeLiteGenerator.cxx index de40c77..bf7555d 100644 --- a/Source/cmExtraCodeLiteGenerator.cxx +++ b/Source/cmExtraCodeLiteGenerator.cxx @@ -204,9 +204,7 @@ std::string cmExtraCodeLiteGenerator::CollectSourceFiles( case cmStateEnums::STATIC_LIBRARY: { projectType = "Static Library"; } break; - case cmStateEnums::SHARED_LIBRARY: { - projectType = "Dynamic Library"; - } break; + case cmStateEnums::SHARED_LIBRARY: case cmStateEnums::MODULE_LIBRARY: { projectType = "Dynamic Library"; } break; diff --git a/Source/cmExtraEclipseCDT4Generator.cxx b/Source/cmExtraEclipseCDT4Generator.cxx index 1c5bcaa..b65f097 100644 --- a/Source/cmExtraEclipseCDT4Generator.cxx +++ b/Source/cmExtraEclipseCDT4Generator.cxx @@ -981,7 +981,6 @@ void cmExtraEclipseCDT4Generator::CreateCProjectFile() const } } break; case cmStateEnums::INTERFACE_LIBRARY: - break; default: break; } diff --git a/Source/cmExtraKateGenerator.cxx b/Source/cmExtraKateGenerator.cxx index 271bbee..de979e2 100644 --- a/Source/cmExtraKateGenerator.cxx +++ b/Source/cmExtraKateGenerator.cxx @@ -129,9 +129,8 @@ void cmExtraKateGenerator::WriteTargets(const cmLocalGenerator& lg, if (targetName == "edit_cache") { const char* editCommand = localGen->GetMakefile()->GetDefinition("CMAKE_EDIT_COMMAND"); - if (editCommand == nullptr) { - insertTarget = false; - } else if (strstr(editCommand, "ccmake") != nullptr) { + if (editCommand == nullptr || + strstr(editCommand, "ccmake") != nullptr) { insertTarget = false; } } diff --git a/Source/cmFindPackageCommand.cxx b/Source/cmFindPackageCommand.cxx index e996327..d1517fe 100644 --- a/Source/cmFindPackageCommand.cxx +++ b/Source/cmFindPackageCommand.cxx @@ -279,9 +279,13 @@ bool cmFindPackageCommand::InitialPass(std::vector const& args) } else if (args[i] == "MODULE") { moduleArgs.insert(i); doing = DoingNone; + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (args[i] == "CONFIG") { configArgs.insert(i); doing = DoingNone; + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (args[i] == "NO_MODULE") { configArgs.insert(i); doing = DoingNone; @@ -318,6 +322,8 @@ bool cmFindPackageCommand::InitialPass(std::vector const& args) this->NoSystemRegistry = true; configArgs.insert(i); doing = DoingNone; + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (args[i] == "NO_CMAKE_BUILDS_PATH") { // Ignore legacy option. configArgs.insert(i); diff --git a/Source/cmGeneratorTarget.cxx b/Source/cmGeneratorTarget.cxx index 611f7b0..93eb8fb 100644 --- a/Source/cmGeneratorTarget.cxx +++ b/Source/cmGeneratorTarget.cxx @@ -1227,7 +1227,6 @@ std::string cmGeneratorTarget::EvaluateInterfaceProperty( return result; case cmGeneratorExpressionDAGChecker::CYCLIC_REFERENCE: // No error. We just skip cyclic references. - return result; case cmGeneratorExpressionDAGChecker::ALREADY_SEEN: // No error. We have already seen this transitive property. return result; @@ -1295,10 +1294,9 @@ std::string AddSwiftInterfaceIncludeDirectories( dag.ReportError(nullptr, "$GetName() + ",Swift_MODULE_DIRECTORY>"); - return ""; + CM_FALLTHROUGH; case cmGeneratorExpressionDAGChecker::CYCLIC_REFERENCE: // No error. We just skip cyclic references. - return ""; case cmGeneratorExpressionDAGChecker::ALREADY_SEEN: // No error. We have already seen this transitive property. return ""; @@ -1689,10 +1687,14 @@ void cmGeneratorTarget::ComputeKindedSources(KindedSources& files, std::string ext = cmSystemTools::LowerCase(sf->GetExtension()); if (sf->GetCustomCommand()) { kind = SourceKindCustomCommand; + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (this->Target->GetType() == cmStateEnums::UTILITY) { kind = SourceKindExtra; } else if (this->IsSourceFilePartOfUnityBatch(sf->ResolveFullPath())) { kind = SourceKindUnityBatched; + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (sf->GetPropertyAsBool("HEADER_FILE_ONLY")) { kind = SourceKindHeader; } else if (sf->GetPropertyAsBool("EXTERNAL_OBJECT")) { diff --git a/Source/cmInstallCommand.cxx b/Source/cmInstallCommand.cxx index 704db66..c9b22b6 100644 --- a/Source/cmInstallCommand.cxx +++ b/Source/cmInstallCommand.cxx @@ -661,7 +661,6 @@ bool HandleTargetsMode(std::vector const& args, // Nothing to do. An INTERFACE_LIBRARY can be installed, but the // only effect of that is to make it exportable. It installs no // other files itself. - break; default: // This should never happen due to the above type check. // Ignore the case. diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx index f5ca5f4..e6083d3 100644 --- a/Source/cmLocalGenerator.cxx +++ b/Source/cmLocalGenerator.cxx @@ -2070,11 +2070,9 @@ bool cmLocalGenerator::GetRealDependency(const std::string& inName, case cmStateEnums::OBJECT_LIBRARY: // An object library has no single file on which to depend. // This was listed to get the target-level dependency. - return false; case cmStateEnums::INTERFACE_LIBRARY: // An interface library has no file on which to depend. // This was listed to get the target-level dependency. - return false; case cmStateEnums::UTILITY: case cmStateEnums::GLOBAL_TARGET: // A utility target has no file on which to depend. This was listed @@ -3389,11 +3387,12 @@ std::string cmLocalGenerator::GetObjectFileNameWithoutTarget( // Select a nice-looking reference to the source file to construct // the object file name. std::string objectName; + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) if ((relSource && !relBinary) || (subSource && !subBinary)) { objectName = relFromSource; - } else if ((relBinary && !relSource) || (subBinary && !subSource)) { - objectName = relFromBinary; - } else if (relFromBinary.length() < relFromSource.length()) { + } else if ((relBinary && !relSource) || (subBinary && !subSource) || + relFromBinary.length() < relFromSource.length()) { objectName = relFromBinary; } else { objectName = relFromSource; @@ -3552,7 +3551,6 @@ bool cmLocalGenerator::NeedBackwardsCompatibility_2_4() break; case cmPolicies::NEW: // New behavior is to ignore the variable. - return false; case cmPolicies::REQUIRED_IF_USED: case cmPolicies::REQUIRED_ALWAYS: // This will never be the case because the only way to require diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 69f316d..0b532e7 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -2513,10 +2513,7 @@ void cmMakefile::ExpandVariablesCMP0019() for (auto l = linkLibs.begin(); l != linkLibs.end(); ++l) { std::string libName = *l; - if (libName == "optimized") { - ++l; - libName = *l; - } else if (libName == "debug") { + if (libName == "optimized" || libName == "debug") { ++l; libName = *l; } diff --git a/Source/cmPolicies.cxx b/Source/cmPolicies.cxx index 5c8bc98..dea3f8a 100644 --- a/Source/cmPolicies.cxx +++ b/Source/cmPolicies.cxx @@ -72,6 +72,7 @@ static const char* idToVersion(cmPolicies::PolicyID id) #define POLICY_CASE(ID, V_MAJOR, V_MINOR, V_PATCH) \ case cmPolicies::ID: \ return #V_MAJOR "." #V_MINOR "." #V_PATCH; + // NOLINTNEXTLINE(bugprone-branch-clone) CM_FOR_EACH_POLICY_ID_VERSION(POLICY_CASE) #undef POLICY_CASE case cmPolicies::CMPCOUNT: @@ -90,6 +91,7 @@ static bool isPolicyNewerThan(cmPolicies::PolicyID id, unsigned int majorV, (majorV == (V_MAJOR) && minorV + 1 < (V_MINOR) + 1) || \ (majorV == (V_MAJOR) && minorV == (V_MINOR) && \ patchV + 1 < (V_PATCH) + 1)); + // NOLINTNEXTLINE(bugprone-branch-clone) CM_FOR_EACH_POLICY_ID_VERSION(POLICY_CASE) #undef POLICY_CASE case cmPolicies::CMPCOUNT: diff --git a/Source/cmRST.cxx b/Source/cmRST.cxx index c39d162..26e93bb 100644 --- a/Source/cmRST.cxx +++ b/Source/cmRST.cxx @@ -166,6 +166,8 @@ void cmRST::ProcessLine(std::string const& line) this->Markup = (line.find_first_not_of(" \t", 2) == std::string::npos ? MarkupEmpty : MarkupNormal); + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) if (this->CMakeDirective.find(line)) { // Output cmake domain directives and their content normally. this->NormalLine(line); diff --git a/Source/cmSeparateArgumentsCommand.cxx b/Source/cmSeparateArgumentsCommand.cxx index 52bde7c..cfe3087 100644 --- a/Source/cmSeparateArgumentsCommand.cxx +++ b/Source/cmSeparateArgumentsCommand.cxx @@ -39,6 +39,8 @@ bool cmSeparateArgumentsCommand(std::vector const& args, if (doing == DoingVariable) { var = arg; doing = DoingMode; + // This will always clone one of the other blocks. + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (doing == DoingMode && arg == "NATIVE_COMMAND") { #ifdef _WIN32 mode = ModeWindows; diff --git a/Source/cmake.cxx b/Source/cmake.cxx index 2ec893f..ea1d8ca 100644 --- a/Source/cmake.cxx +++ b/Source/cmake.cxx @@ -642,6 +642,8 @@ void cmake::SetArgs(const std::vector& args) path = cmSystemTools::CollapseFullPath(path); cmSystemTools::ConvertToUnixSlashes(path); this->SetHomeDirectory(path); + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (cmHasLiteralPrefix(arg, "-O")) { // There is no local generate anymore. Ignore -O option. } else if (cmHasLiteralPrefix(arg, "-B")) { @@ -681,27 +683,16 @@ void cmake::SetArgs(const std::vector& args) this->VSSolutionFile = args[++i]; } #endif - else if (cmHasLiteralPrefix(arg, "-D")) { + else if (cmHasLiteralPrefix(arg, "-D") || cmHasLiteralPrefix(arg, "-U") || + cmHasLiteralPrefix(arg, "-C")) { // skip for now - // in case '-D var=val' is given, also skip the next - // in case '-Dvar=val' is given, don't skip the next - if (arg.size() == 2) { - ++i; - } - } else if (cmHasLiteralPrefix(arg, "-U")) { - // skip for now - // in case '-U var' is given, also skip the next - // in case '-Uvar' is given, don't skip the next - if (arg.size() == 2) { - ++i; - } - } else if (cmHasLiteralPrefix(arg, "-C")) { - // skip for now - // in case '-C path' is given, also skip the next - // in case '-Cpath' is given, don't skip the next + // in case '-[DUC] argval' var' is given, also skip the next + // in case '-[DUC]argval' is given, don't skip the next if (arg.size() == 2) { ++i; } + // XXX(clang-tidy): https://bugs.llvm.org/show_bug.cgi?id=44165 + // NOLINTNEXTLINE(bugprone-branch-clone) } else if (cmHasLiteralPrefix(arg, "-P")) { // skip for now i++; diff --git a/Source/cmcmd.cxx b/Source/cmcmd.cxx index c3bd160..49d04a6 100644 --- a/Source/cmcmd.cxx +++ b/Source/cmcmd.cxx @@ -1972,9 +1972,8 @@ bool cmVSLink::Parse(std::vector::const_iterator argBeg, // Parse the link command to extract information we need. for (; arg != argEnd; ++arg) { - if (cmSystemTools::Strucmp(arg->c_str(), "/INCREMENTAL:YES") == 0) { - this->Incremental = true; - } else if (cmSystemTools::Strucmp(arg->c_str(), "/INCREMENTAL") == 0) { + if (cmSystemTools::Strucmp(arg->c_str(), "/INCREMENTAL:YES") == 0 || + cmSystemTools::Strucmp(arg->c_str(), "/INCREMENTAL") == 0) { this->Incremental = true; } else if (cmSystemTools::Strucmp(arg->c_str(), "/MANIFEST:NO") == 0) { this->LinkGeneratesManifest = false; -- cgit v0.12