Index: clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h =================================================================== --- clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h +++ clang-apply-replacements/include/clang-apply-replacements/Tooling/ApplyReplacements.h @@ -91,6 +91,11 @@ FileToReplacementsMap &GroupedReplacements, clang::SourceManager &SM); +// FIXME: Remove this function after changing clang-apply-replacements to use +// Replacements class. +bool applyAllReplacements(const std::vector &Replaces, + Rewriter &Rewrite); + /// \brief Apply all replacements in \c GroupedReplacements. /// /// \param[in] GroupedReplacements Deduplicated and conflict free Replacements Index: clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp =================================================================== --- clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp +++ clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp @@ -122,6 +122,81 @@ } } +// FIXME: Remove this function after changing clang-apply-replacements to use +// Replacements class. +bool applyAllReplacements(const std::vector &Replaces, + Rewriter &Rewrite) { + bool Result = true; + for (std::vector::const_iterator I = Replaces.begin(), + E = Replaces.end(); + I != E; ++I) { + if (I->isApplicable()) { + Result = I->apply(Rewrite) && Result; + } else { + Result = false; + } + } + return Result; +} + + +// FIXME: moved from libToolingCore. remove this when std::vector +// is replaced with tooling::Replacements class. +static void deduplicate(std::vector &Replaces, + std::vector &Conflicts) { + if (Replaces.empty()) + return; + + auto LessNoPath = [](const tooling::Replacement &LHS, + const tooling::Replacement &RHS) { + if (LHS.getOffset() != RHS.getOffset()) + return LHS.getOffset() < RHS.getOffset(); + if (LHS.getLength() != RHS.getLength()) + return LHS.getLength() < RHS.getLength(); + return LHS.getReplacementText() < RHS.getReplacementText(); + }; + + auto EqualNoPath = [](const tooling::Replacement &LHS, + const tooling::Replacement &RHS) { + return LHS.getOffset() == RHS.getOffset() && + LHS.getLength() == RHS.getLength() && + LHS.getReplacementText() == RHS.getReplacementText(); + }; + + // Deduplicate. We don't want to deduplicate based on the path as we assume + // that all replacements refer to the same file (or are symlinks). + std::sort(Replaces.begin(), Replaces.end(), LessNoPath); + Replaces.erase(std::unique(Replaces.begin(), Replaces.end(), EqualNoPath), + Replaces.end()); + + // Detect conflicts + tooling::Range ConflictRange(Replaces.front().getOffset(), + Replaces.front().getLength()); + unsigned ConflictStart = 0; + unsigned ConflictLength = 1; + for (unsigned i = 1; i < Replaces.size(); ++i) { + tooling::Range Current(Replaces[i].getOffset(), Replaces[i].getLength()); + if (ConflictRange.overlapsWith(Current)) { + // Extend conflicted range + ConflictRange = + tooling::Range(ConflictRange.getOffset(), + std::max(ConflictRange.getLength(), + Current.getOffset() + Current.getLength() - + ConflictRange.getOffset())); + ++ConflictLength; + } else { + if (ConflictLength > 1) + Conflicts.push_back(tooling::Range(ConflictStart, ConflictLength)); + ConflictRange = Current; + ConflictStart = i; + ConflictLength = 1; + } + } + + if (ConflictLength > 1) + Conflicts.push_back(tooling::Range(ConflictStart, ConflictLength)); +} + /// \brief Deduplicates and tests for conflicts among the replacements for each /// file in \c Replacements. Any conflicts found are reported. /// @@ -144,7 +219,7 @@ assert(Entry != nullptr && "No file entry!"); std::vector Conflicts; - tooling::deduplicate(FileAndReplacements.second, Conflicts); + deduplicate(FileAndReplacements.second, Conflicts); if (Conflicts.empty()) continue; @@ -197,7 +272,7 @@ // However, until we nail down the design of ReplacementGroups, might as well // leave this as is. for (const auto &FileAndReplacements : GroupedReplacements) { - if (!tooling::applyAllReplacements(FileAndReplacements.second, Rewrites)) + if (!applyAllReplacements(FileAndReplacements.second, Rewrites)) return false; } Index: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp =================================================================== --- clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp +++ clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp @@ -108,7 +108,7 @@ Rewriter &Rewrites, std::string &Result) { if (Replacements.empty()) return true; - if (!tooling::applyAllReplacements(Replacements, Rewrites)) + if (!applyAllReplacements(Replacements, Rewrites)) return false; SourceManager &SM = Rewrites.getSourceMgr(); Index: clang-rename/RenamingAction.h =================================================================== --- clang-rename/RenamingAction.h +++ clang-rename/RenamingAction.h @@ -27,17 +27,17 @@ public: RenamingAction(const std::string &NewName, const std::string &PrevName, const std::vector &USRs, - tooling::Replacements &Replaces, bool PrintLocations = false) - : NewName(NewName), PrevName(PrevName), USRs(USRs), Replaces(Replaces), - PrintLocations(PrintLocations) { - } + std::map &FileToReplaces, + bool PrintLocations = false) + : NewName(NewName), PrevName(PrevName), USRs(USRs), + FileToReplaces(FileToReplaces), PrintLocations(PrintLocations) {} std::unique_ptr newASTConsumer(); private: const std::string &NewName, &PrevName; const std::vector &USRs; - tooling::Replacements &Replaces; + std::map &FileToReplaces; bool PrintLocations; }; Index: clang-rename/RenamingAction.cpp =================================================================== --- clang-rename/RenamingAction.cpp +++ clang-rename/RenamingAction.cpp @@ -34,14 +34,13 @@ class RenamingASTConsumer : public ASTConsumer { public: - RenamingASTConsumer(const std::string &NewName, - const std::string &PrevName, - const std::vector &USRs, - tooling::Replacements &Replaces, - bool PrintLocations) - : NewName(NewName), PrevName(PrevName), USRs(USRs), Replaces(Replaces), - PrintLocations(PrintLocations) { - } + RenamingASTConsumer( + const std::string &NewName, const std::string &PrevName, + const std::vector &USRs, + std::map &FileToReplaces, + bool PrintLocations) + : NewName(NewName), PrevName(PrevName), USRs(USRs), + FileToReplaces(FileToReplaces), PrintLocations(PrintLocations) {} void HandleTranslationUnit(ASTContext &Context) override { const auto &SourceMgr = Context.getSourceManager(); @@ -63,25 +62,35 @@ errs() << "clang-rename: renamed at: " << SourceMgr.getFilename(Loc) << ":" << FullLoc.getSpellingLineNumber() << ":" << FullLoc.getSpellingColumnNumber() << "\n"; - Replaces.insert(tooling::Replacement(SourceMgr, Loc, PrevNameLen, - NewName)); + // FIXME: better error handling. + auto Replace = + tooling::Replacement(SourceMgr, Loc, PrevNameLen, NewName); + auto Err = FileToReplaces[Replace.getFilePath()].add(Replace); + if (Err) + llvm::errs() << "Renaming failed in " << Replace.getFilePath() << "! " + << llvm::toString(std::move(Err)) << "\n"; } else - for (const auto &Loc : RenamingCandidates) - Replaces.insert(tooling::Replacement(SourceMgr, Loc, PrevNameLen, - NewName)); + for (const auto &Loc : RenamingCandidates) { + auto Replace = + tooling::Replacement(SourceMgr, Loc, PrevNameLen, NewName); + auto Err = FileToReplaces[Replace.getFilePath()].add(Replace); + if (Err) + llvm::errs() << "Renaming failed in " << Replace.getFilePath() << "! " + << llvm::toString(std::move(Err)) << "\n"; + } } private: const std::string &NewName, &PrevName; const std::vector &USRs; - tooling::Replacements &Replaces; + std::map &FileToReplaces; bool PrintLocations; }; std::unique_ptr RenamingAction::newASTConsumer() { return llvm::make_unique(NewName, PrevName, USRs, - Replaces, PrintLocations); + FileToReplaces, PrintLocations); } } // namespace rename Index: clang-rename/tool/ClangRename.cpp =================================================================== --- clang-rename/tool/ClangRename.cpp +++ clang-rename/tool/ClangRename.cpp @@ -143,9 +143,10 @@ // Export replacements. tooling::TranslationUnitReplacements TUR; - const tooling::Replacements &Replacements = Tool.getReplacements(); - TUR.Replacements.insert(TUR.Replacements.end(), Replacements.begin(), - Replacements.end()); + const auto &FileToReplacements = Tool.getReplacements(); + for (const auto &Entry : FileToReplacements) + TUR.Replacements.insert(TUR.Replacements.end(), Entry.second.begin(), + Entry.second.end()); yaml::Output YAML(OS); YAML << TUR; Index: clang-tidy/ClangTidyDiagnosticConsumer.cpp =================================================================== --- clang-tidy/ClangTidyDiagnosticConsumer.cpp +++ clang-tidy/ClangTidyDiagnosticConsumer.cpp @@ -77,7 +77,14 @@ assert(Range.getBegin().isFileID() && Range.getEnd().isFileID() && "Only file locations supported in fix-it hints."); - Error.Fix.insert(tooling::Replacement(SM, Range, FixIt.CodeToInsert)); + auto Err = + Error.Fix.add(tooling::Replacement(SM, Range, FixIt.CodeToInsert)); + // FIXME: better error handling. + if (Err) { + llvm::errs() << "Fix conflicts with existing fix! " + << llvm::toString(std::move(Err)) << "\n"; + } + assert(!Err && "Fix conflicts with existing fix!"); } } Index: include-fixer/IncludeFixer.cpp =================================================================== --- include-fixer/IncludeFixer.cpp +++ include-fixer/IncludeFixer.cpp @@ -357,8 +357,11 @@ return tooling::Replacements(); std::string IncludeName = "#include " + Header.str() + "\n"; // Create replacements for the new header. - clang::tooling::Replacements Insertions = { - tooling::Replacement(FilePath, UINT_MAX, 0, IncludeName)}; + clang::tooling::Replacements Insertions; + // FIXME: remove this error comsuption when createInsertHeaderReplacements + // returns Error. + llvm::consumeError( + Insertions.add(tooling::Replacement(FilePath, UINT_MAX, 0, IncludeName))); auto CleanReplaces = cleanupAroundReplacements(Code, Insertions, Style); if (!CleanReplaces) Index: include-fixer/tool/ClangIncludeFixer.cpp =================================================================== --- include-fixer/tool/ClangIncludeFixer.cpp +++ include-fixer/tool/ClangIncludeFixer.cpp @@ -262,12 +262,14 @@ return 1; } - auto Replacements = clang::include_fixer::createInsertHeaderReplacements( + // FIXME: pull code for generating header insertion and namespace insertion + // replacements into a function since similar stuff is performed below. + auto ReplacesOrError = clang::include_fixer::createInsertHeaderReplacements( Code->getBuffer(), FilePath, Context.getHeaderInfos().front().Header, InsertStyle); - if (!Replacements) { + if (!ReplacesOrError) { errs() << "Failed to create header insertion replacement: " - << llvm::toString(Replacements.takeError()) << "\n"; + << llvm::toString(ReplacesOrError.takeError()) << "\n"; return 1; } @@ -281,12 +283,23 @@ const IncludeFixerContext::HeaderInfo &RHS) { return LHS.QualifiedName == RHS.QualifiedName; }); - if (IsUniqueQualifiedName) - Replacements->insert({FilePath, Context.getSymbolRange().getOffset(), - Context.getSymbolRange().getLength(), - Context.getHeaderInfos().front().QualifiedName}); + auto Replaces = std::move(*ReplacesOrError); + if (IsUniqueQualifiedName) { + auto R = + tooling::Replacement(FilePath, Context.getSymbolRange().getOffset(), + Context.getSymbolRange().getLength(), + Context.getHeaderInfos().front().QualifiedName); + auto Err = Replaces.add(R); + if (Err) { + llvm::consumeError(std::move(Err)); + R = tooling::Replacement(R.getFilePath(), + Replaces.getShiftedCodePosition(R.getOffset()), + R.getLength(), R.getReplacementText()); + Replaces = Replaces.merge(tooling::Replacements(R)); + } + } auto ChangedCode = - tooling::applyAllReplacements(Code->getBuffer(), *Replacements); + tooling::applyAllReplacements(Code->getBuffer(), Replaces); if (!ChangedCode) { llvm::errs() << llvm::toString(ChangedCode.takeError()) << "\n"; return 1; @@ -326,22 +339,32 @@ return 1; } - auto Replacements = clang::include_fixer::createInsertHeaderReplacements( + // FIXME: Rank the results and pick the best one instead of the first one. + auto ReplacesOrError = clang::include_fixer::createInsertHeaderReplacements( /*Code=*/Buffer.get()->getBuffer(), FilePath, Context.getHeaderInfos().front().Header, InsertStyle); - if (!Replacements) { + if (!ReplacesOrError) { errs() << "Failed to create header insertion replacement: " - << llvm::toString(Replacements.takeError()) << "\n"; + << llvm::toString(ReplacesOrError.takeError()) << "\n"; return 1; } if (!Quiet) llvm::errs() << "Added #include" << Context.getHeaderInfos().front().Header; + auto Replaces = std::move(*ReplacesOrError); // Add missing namespace qualifiers to the unidentified symbol. - Replacements->insert({FilePath, Context.getSymbolRange().getOffset(), - Context.getSymbolRange().getLength(), - Context.getHeaderInfos().front().QualifiedName}); + auto R = tooling::Replacement(FilePath, Context.getSymbolRange().getOffset(), + Context.getSymbolRange().getLength(), + Context.getHeaderInfos().front().QualifiedName); + auto Err = Replaces.add(R); + if (Err) { + llvm::consumeError(std::move(Err)); + R = tooling::Replacement(R.getFilePath(), + Replaces.getShiftedCodePosition(R.getOffset()), + R.getLength(), R.getReplacementText()); + Replaces = Replaces.merge(tooling::Replacements(R)); + } // Set up a new source manager for applying the resulting replacements. IntrusiveRefCntPtr DiagOpts(new DiagnosticOptions); @@ -352,7 +375,7 @@ if (STDINMode) { auto ChangedCode = - tooling::applyAllReplacements(Code->getBuffer(), *Replacements); + tooling::applyAllReplacements(Code->getBuffer(), Replaces); if (!ChangedCode) { llvm::errs() << llvm::toString(ChangedCode.takeError()) << "\n"; return 1; @@ -363,7 +386,7 @@ // Write replacements to disk. Rewriter Rewrites(SM, LangOptions()); - tooling::applyAllReplacements(*Replacements, Rewrites); + tooling::applyAllReplacements(Replaces, Rewrites); return Rewrites.overwriteChangedFiles(); } Index: tool-template/ToolTemplate.cpp =================================================================== --- tool-template/ToolTemplate.cpp +++ tool-template/ToolTemplate.cpp @@ -53,18 +53,20 @@ namespace { class ToolTemplateCallback : public MatchFinder::MatchCallback { - public: - ToolTemplateCallback(Replacements *Replace) : Replace(Replace) {} +public: + ToolTemplateCallback(std::map *Replace) + : Replace(Replace) {} void run(const MatchFinder::MatchResult &Result) override { - // TODO: This routine will get called for each thing that the matchers find. + // TODO: This routine will get called for each thing that the matchers + // find. // At this point, you can examine the match, and do whatever you want, // including replacing the matched text with other text (void)Replace; // This to prevent an "unused member variable" warning; } - private: - Replacements *Replace; +private: + std::map *Replace; }; } // end anonymous namespace Index: unittests/clang-tidy/ClangTidyTest.h =================================================================== --- unittests/clang-tidy/ClangTidyTest.h +++ unittests/clang-tidy/ClangTidyTest.h @@ -119,7 +119,14 @@ DiagConsumer.finish(); tooling::Replacements Fixes; for (const ClangTidyError &Error : Context.getErrors()) - Fixes.insert(Error.Fix.begin(), Error.Fix.end()); + for (const auto &Fix : Error.Fix) { + auto Err = Fixes.add(Fix); + // FIXME: better error handling. + if (Err) { + llvm::errs() << llvm::toString(std::move(Err)) << "\n"; + return ""; + } + } if (Errors) *Errors = Context.getErrors(); auto Result = tooling::applyAllReplacements(Code, Fixes); Index: unittests/include-fixer/IncludeFixerTest.cpp =================================================================== --- unittests/include-fixer/IncludeFixerTest.cpp +++ unittests/include-fixer/IncludeFixerTest.cpp @@ -89,18 +89,28 @@ runOnCode(&Factory, Code, FakeFileName, ExtraArgs); if (FixerContext.getHeaderInfos().empty()) return Code; - auto Replaces = clang::include_fixer::createInsertHeaderReplacements( + auto ReplacesOrError = clang::include_fixer::createInsertHeaderReplacements( Code, FakeFileName, FixerContext.getHeaderInfos().front().Header); - EXPECT_TRUE(static_cast(Replaces)) - << llvm::toString(Replaces.takeError()) << "\n"; - if (!Replaces) + EXPECT_TRUE(static_cast(ReplacesOrError)) + << llvm::toString(ReplacesOrError.takeError()) << "\n"; + if (!ReplacesOrError) return ""; + auto Replaces = std::move(*ReplacesOrError); + auto R = tooling::Replacement( + FakeFileName, FixerContext.getSymbolRange().getOffset(), + FixerContext.getSymbolRange().getLength(), + FixerContext.getHeaderInfos().front().QualifiedName); + auto Err = Replaces.add(R); + if (Err) { + llvm::consumeError(std::move(Err)); + R = tooling::Replacement(R.getFilePath(), + Replaces.getShiftedCodePosition(R.getOffset()), + R.getLength(), R.getReplacementText()); + Replaces = Replaces.merge(tooling::Replacements(R)); + } clang::RewriterTestContext Context; clang::FileID ID = Context.createInMemoryFile(FakeFileName, Code); - Replaces->insert({FakeFileName, FixerContext.getSymbolRange().getOffset(), - FixerContext.getSymbolRange().getLength(), - FixerContext.getHeaderInfos().front().QualifiedName}); - clang::tooling::applyAllReplacements(*Replaces, Context.Rewrite); + EXPECT_TRUE(clang::tooling::applyAllReplacements(Replaces, Context.Rewrite)); return Context.getRewrittenText(ID); }