[PATCH] D25613: [clang-move] Don't overuse Replacements::add.
This revision was automatically updated to reflect the committed changes. hokein marked an inline comment as done. Closed by commit rL284236: [clang-move] Don't overuse Replacements::add. (authored by hokein). Changed prior to commit: https://reviews.llvm.org/D25613?vs=74677&id=74679#toc Repository: rL LLVM https://reviews.llvm.org/D25613 Files: clang-tools-extra/trunk/clang-move/ClangMove.cpp Index: clang-tools-extra/trunk/clang-move/ClangMove.cpp === --- clang-tools-extra/trunk/clang-move/ClangMove.cpp +++ clang-tools-extra/trunk/clang-move/ClangMove.cpp @@ -193,27 +193,6 @@ return SourceText.str(); } -clang::tooling::Replacement -getReplacementInChangedCode(const clang::tooling::Replacements &Replacements, -const clang::tooling::Replacement &Replacement) { - unsigned Start = Replacements.getShiftedCodePosition(Replacement.getOffset()); - unsigned End = Replacements.getShiftedCodePosition(Replacement.getOffset() + - Replacement.getLength()); - return clang::tooling::Replacement(Replacement.getFilePath(), Start, - End - Start, - Replacement.getReplacementText()); -} - -void addOrMergeReplacement(const clang::tooling::Replacement &Replacement, - clang::tooling::Replacements *Replacements) { - auto Err = Replacements->add(Replacement); - if (Err) { -llvm::consumeError(std::move(Err)); -auto Replace = getReplacementInChangedCode(*Replacements, Replacement); -*Replacements = Replacements->merge(clang::tooling::Replacements(Replace)); - } -} - bool isInHeaderFile(const clang::SourceManager &SM, const clang::Decl *D, llvm::StringRef OriginalRunningDirectory, llvm::StringRef OldHeader) { @@ -251,32 +230,24 @@ const std::vector &Decls, llvm::StringRef FileName, bool IsHeader = false) { - clang::tooling::Replacements InsertedReplacements; + std::string NewCode; std::string GuardName(FileName); if (IsHeader) { std::replace(GuardName.begin(), GuardName.end(), '/', '_'); std::replace(GuardName.begin(), GuardName.end(), '.', '_'); std::replace(GuardName.begin(), GuardName.end(), '-', '_'); GuardName = StringRef(GuardName).upper(); -std::string HeaderGuard = "#ifndef " + GuardName + "\n"; -HeaderGuard += "#define " + GuardName + "\n"; -clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0, - HeaderGuard); -addOrMergeReplacement(HeaderGuardInclude, &InsertedReplacements); +NewCode += "#ifndef " + GuardName + "\n"; +NewCode += "#define " + GuardName + "\n"; } // Add #Includes. - std::string AllIncludesString; - // FIXME: Add header guard. for (const auto &Include : Includes) -AllIncludesString += Include; +NewCode += Include; - if (!AllIncludesString.empty()) { -clang::tooling::Replacement InsertInclude(FileName, 0, 0, - AllIncludesString + "\n"); -addOrMergeReplacement(InsertInclude, &InsertedReplacements); - } + if (!Includes.empty()) +NewCode += "\n"; // Add moved class definition and its related declarations. All declarations // in same namespace are grouped together. @@ -299,36 +270,23 @@ for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0; --RemainingSize, ++It) { assert(It < CurrentNamespaces.rend()); - auto code = "} // namespace " + *It + "\n"; - clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "} // namespace " + *It + "\n"; } while (DeclIt != DeclNamespaces.end()) { - clang::tooling::Replacement InsertedReplacement( - FileName, 0, 0, "namespace " + *DeclIt + " {\n"); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "namespace " + *DeclIt + " {\n"; ++DeclIt; } - -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM)); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - +NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM); CurrentNamespaces = std::move(NextNamespaces); } std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end()); - for (const auto &NS : CurrentNamespaces) { -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, "} // namespace " + NS + "\n"); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - } + for (const auto &NS : CurrentNamespaces) +NewCode += "} // namespace " + NS + "\n"; -
[PATCH] D25613: [clang-move] Don't overuse Replacements::add.
ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land. Awesome! Lg with one nit. Comment at: clang-move/ClangMove.cpp:197 clang::tooling::Replacement getReplacementInChangedCode(const clang::tooling::Replacements &Replacements, const clang::tooling::Replacement &Replacement) { This is also dead I think :P https://reviews.llvm.org/D25613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25613: [clang-move] Don't overuse Replacements::add.
hokein updated this revision to Diff 74677. hokein added a comment. Delete one more function. https://reviews.llvm.org/D25613 Files: clang-move/ClangMove.cpp Index: clang-move/ClangMove.cpp === --- clang-move/ClangMove.cpp +++ clang-move/ClangMove.cpp @@ -193,27 +193,6 @@ return SourceText.str(); } -clang::tooling::Replacement -getReplacementInChangedCode(const clang::tooling::Replacements &Replacements, -const clang::tooling::Replacement &Replacement) { - unsigned Start = Replacements.getShiftedCodePosition(Replacement.getOffset()); - unsigned End = Replacements.getShiftedCodePosition(Replacement.getOffset() + - Replacement.getLength()); - return clang::tooling::Replacement(Replacement.getFilePath(), Start, - End - Start, - Replacement.getReplacementText()); -} - -void addOrMergeReplacement(const clang::tooling::Replacement &Replacement, - clang::tooling::Replacements *Replacements) { - auto Err = Replacements->add(Replacement); - if (Err) { -llvm::consumeError(std::move(Err)); -auto Replace = getReplacementInChangedCode(*Replacements, Replacement); -*Replacements = Replacements->merge(clang::tooling::Replacements(Replace)); - } -} - bool isInHeaderFile(const clang::SourceManager &SM, const clang::Decl *D, llvm::StringRef OriginalRunningDirectory, llvm::StringRef OldHeader) { @@ -251,32 +230,24 @@ const std::vector &Decls, llvm::StringRef FileName, bool IsHeader = false) { - clang::tooling::Replacements InsertedReplacements; + std::string NewCode; std::string GuardName(FileName); if (IsHeader) { std::replace(GuardName.begin(), GuardName.end(), '/', '_'); std::replace(GuardName.begin(), GuardName.end(), '.', '_'); std::replace(GuardName.begin(), GuardName.end(), '-', '_'); GuardName = StringRef(GuardName).upper(); -std::string HeaderGuard = "#ifndef " + GuardName + "\n"; -HeaderGuard += "#define " + GuardName + "\n"; -clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0, - HeaderGuard); -addOrMergeReplacement(HeaderGuardInclude, &InsertedReplacements); +NewCode += "#ifndef " + GuardName + "\n"; +NewCode += "#define " + GuardName + "\n"; } // Add #Includes. - std::string AllIncludesString; - // FIXME: Add header guard. for (const auto &Include : Includes) -AllIncludesString += Include; +NewCode += Include; - if (!AllIncludesString.empty()) { -clang::tooling::Replacement InsertInclude(FileName, 0, 0, - AllIncludesString + "\n"); -addOrMergeReplacement(InsertInclude, &InsertedReplacements); - } + if (!Includes.empty()) +NewCode += "\n"; // Add moved class definition and its related declarations. All declarations // in same namespace are grouped together. @@ -299,36 +270,23 @@ for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0; --RemainingSize, ++It) { assert(It < CurrentNamespaces.rend()); - auto code = "} // namespace " + *It + "\n"; - clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "} // namespace " + *It + "\n"; } while (DeclIt != DeclNamespaces.end()) { - clang::tooling::Replacement InsertedReplacement( - FileName, 0, 0, "namespace " + *DeclIt + " {\n"); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "namespace " + *DeclIt + " {\n"; ++DeclIt; } - -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM)); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - +NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM); CurrentNamespaces = std::move(NextNamespaces); } std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end()); - for (const auto &NS : CurrentNamespaces) { -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, "} // namespace " + NS + "\n"); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - } + for (const auto &NS : CurrentNamespaces) +NewCode += "} // namespace " + NS + "\n"; - if (IsHeader) { -clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0, - "#endif // " + GuardName + "\n"); -addOrMergeReplacement(HeaderGuardEnd, &InsertedReplacements); - } - return InsertedReplacements; + if (IsHeader) +NewCode += "#endif // " + G
[PATCH] D25613: [clang-move] Don't overuse Replacements::add.
hokein updated this revision to Diff 74675. hokein added a comment. woohoo, delete more code! https://reviews.llvm.org/D25613 Files: clang-move/ClangMove.cpp Index: clang-move/ClangMove.cpp === --- clang-move/ClangMove.cpp +++ clang-move/ClangMove.cpp @@ -204,16 +204,6 @@ Replacement.getReplacementText()); } -void addOrMergeReplacement(const clang::tooling::Replacement &Replacement, - clang::tooling::Replacements *Replacements) { - auto Err = Replacements->add(Replacement); - if (Err) { -llvm::consumeError(std::move(Err)); -auto Replace = getReplacementInChangedCode(*Replacements, Replacement); -*Replacements = Replacements->merge(clang::tooling::Replacements(Replace)); - } -} - bool isInHeaderFile(const clang::SourceManager &SM, const clang::Decl *D, llvm::StringRef OriginalRunningDirectory, llvm::StringRef OldHeader) { @@ -251,32 +241,24 @@ const std::vector &Decls, llvm::StringRef FileName, bool IsHeader = false) { - clang::tooling::Replacements InsertedReplacements; + std::string NewCode; std::string GuardName(FileName); if (IsHeader) { std::replace(GuardName.begin(), GuardName.end(), '/', '_'); std::replace(GuardName.begin(), GuardName.end(), '.', '_'); std::replace(GuardName.begin(), GuardName.end(), '-', '_'); GuardName = StringRef(GuardName).upper(); -std::string HeaderGuard = "#ifndef " + GuardName + "\n"; -HeaderGuard += "#define " + GuardName + "\n"; -clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0, - HeaderGuard); -addOrMergeReplacement(HeaderGuardInclude, &InsertedReplacements); +NewCode += "#ifndef " + GuardName + "\n"; +NewCode += "#define " + GuardName + "\n"; } // Add #Includes. - std::string AllIncludesString; - // FIXME: Add header guard. for (const auto &Include : Includes) -AllIncludesString += Include; +NewCode += Include; - if (!AllIncludesString.empty()) { -clang::tooling::Replacement InsertInclude(FileName, 0, 0, - AllIncludesString + "\n"); -addOrMergeReplacement(InsertInclude, &InsertedReplacements); - } + if (!Includes.empty()) +NewCode += "\n"; // Add moved class definition and its related declarations. All declarations // in same namespace are grouped together. @@ -299,36 +281,23 @@ for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0; --RemainingSize, ++It) { assert(It < CurrentNamespaces.rend()); - auto code = "} // namespace " + *It + "\n"; - clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "} // namespace " + *It + "\n"; } while (DeclIt != DeclNamespaces.end()) { - clang::tooling::Replacement InsertedReplacement( - FileName, 0, 0, "namespace " + *DeclIt + " {\n"); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "namespace " + *DeclIt + " {\n"; ++DeclIt; } - -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM)); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - +NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM); CurrentNamespaces = std::move(NextNamespaces); } std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end()); - for (const auto &NS : CurrentNamespaces) { -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, "} // namespace " + NS + "\n"); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - } + for (const auto &NS : CurrentNamespaces) +NewCode += "} // namespace " + NS + "\n"; - if (IsHeader) { -clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0, - "#endif // " + GuardName + "\n"); -addOrMergeReplacement(HeaderGuardEnd, &InsertedReplacements); - } - return InsertedReplacements; + if (IsHeader) +NewCode += "#endif // " + GuardName + "\n"; + return clang::tooling::Replacements( + clang::tooling::Replacement(FileName, 0, 0, NewCode)); } } // namespace @@ -490,7 +459,11 @@ Range.getBegin(), Range.getEnd()), ""); std::string FilePath = RemoveReplacement.getFilePath().str(); -addOrMergeReplacement(RemoveReplacement, &FileToReplacements[FilePath]); +auto Err = FileToReplacements[FilePath].add(RemoveReplacement); +if (Err) { + llvm::errs() << llvm::toString(std::move(Err)) << "\n"; + continue; +} llvm::StringRef Code =
[PATCH] D25613: [clang-move] Don't overuse Replacements::add.
ioeric added inline comments. Comment at: clang-move/ClangMove.cpp:472 std::string FilePath = RemoveReplacement.getFilePath().str(); addOrMergeReplacement(RemoveReplacement, &FileToReplacements[FilePath]); For deletions, you can simply use `add`, which now simply merges overlapping deletions. And I think it should be considered an error if `add` fails. With `add`, you can get rid of `addOrMergeReplacement`. https://reviews.llvm.org/D25613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25613: [clang-move] Don't overuse Replacements::add.
hokein created this revision. hokein added a reviewer: ioeric. hokein added a subscriber: cfe-commits. https://reviews.llvm.org/D25613 Files: clang-move/ClangMove.cpp Index: clang-move/ClangMove.cpp === --- clang-move/ClangMove.cpp +++ clang-move/ClangMove.cpp @@ -251,32 +251,24 @@ const std::vector &Decls, llvm::StringRef FileName, bool IsHeader = false) { - clang::tooling::Replacements InsertedReplacements; + std::string NewCode; std::string GuardName(FileName); if (IsHeader) { std::replace(GuardName.begin(), GuardName.end(), '/', '_'); std::replace(GuardName.begin(), GuardName.end(), '.', '_'); std::replace(GuardName.begin(), GuardName.end(), '-', '_'); GuardName = StringRef(GuardName).upper(); -std::string HeaderGuard = "#ifndef " + GuardName + "\n"; -HeaderGuard += "#define " + GuardName + "\n"; -clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0, - HeaderGuard); -addOrMergeReplacement(HeaderGuardInclude, &InsertedReplacements); +NewCode += "#ifndef " + GuardName + "\n"; +NewCode += "#define " + GuardName + "\n"; } // Add #Includes. - std::string AllIncludesString; - // FIXME: Add header guard. for (const auto &Include : Includes) -AllIncludesString += Include; +NewCode += Include; - if (!AllIncludesString.empty()) { -clang::tooling::Replacement InsertInclude(FileName, 0, 0, - AllIncludesString + "\n"); -addOrMergeReplacement(InsertInclude, &InsertedReplacements); - } + if (!Includes.empty()) +NewCode += "\n"; // Add moved class definition and its related declarations. All declarations // in same namespace are grouped together. @@ -299,36 +291,23 @@ for (auto It = CurrentNamespaces.rbegin(); RemainingSize > 0; --RemainingSize, ++It) { assert(It < CurrentNamespaces.rend()); - auto code = "} // namespace " + *It + "\n"; - clang::tooling::Replacement InsertedReplacement(FileName, 0, 0, code); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "} // namespace " + *It + "\n"; } while (DeclIt != DeclNamespaces.end()) { - clang::tooling::Replacement InsertedReplacement( - FileName, 0, 0, "namespace " + *DeclIt + " {\n"); - addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); + NewCode += "namespace " + *DeclIt + " {\n"; ++DeclIt; } - -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM)); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - +NewCode += getDeclarationSourceText(MovedDecl.Decl, MovedDecl.SM); CurrentNamespaces = std::move(NextNamespaces); } std::reverse(CurrentNamespaces.begin(), CurrentNamespaces.end()); - for (const auto &NS : CurrentNamespaces) { -clang::tooling::Replacement InsertedReplacement( -FileName, 0, 0, "} // namespace " + NS + "\n"); -addOrMergeReplacement(InsertedReplacement, &InsertedReplacements); - } + for (const auto &NS : CurrentNamespaces) +NewCode += "} // namespace " + NS + "\n"; - if (IsHeader) { -clang::tooling::Replacement HeaderGuardEnd(FileName, 0, 0, - "#endif // " + GuardName + "\n"); -addOrMergeReplacement(HeaderGuardEnd, &InsertedReplacements); - } - return InsertedReplacements; + if (IsHeader) +NewCode += "#endif // " + GuardName + "\n"; + return clang::tooling::Replacements( + clang::tooling::Replacement(FileName, 0, 0, NewCode)); } } // namespace Index: clang-move/ClangMove.cpp === --- clang-move/ClangMove.cpp +++ clang-move/ClangMove.cpp @@ -251,32 +251,24 @@ const std::vector &Decls, llvm::StringRef FileName, bool IsHeader = false) { - clang::tooling::Replacements InsertedReplacements; + std::string NewCode; std::string GuardName(FileName); if (IsHeader) { std::replace(GuardName.begin(), GuardName.end(), '/', '_'); std::replace(GuardName.begin(), GuardName.end(), '.', '_'); std::replace(GuardName.begin(), GuardName.end(), '-', '_'); GuardName = StringRef(GuardName).upper(); -std::string HeaderGuard = "#ifndef " + GuardName + "\n"; -HeaderGuard += "#define " + GuardName + "\n"; -clang::tooling::Replacement HeaderGuardInclude(FileName, 0, 0, - HeaderGuard); -addOrMergeReplacement(HeaderGuardInclude, &InsertedReplacements); +NewCode += "#ifndef " + GuardName + "\n"; +NewCode += "#define " + GuardName + "\n"; }