On Mon, Feb 29, 2016 at 5:39 PM Chandler Carruth <chandl...@gmail.com> wrote:
> On Mon, Feb 29, 2016 at 11:32 AM Manuel Klimek via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > >> Author: klimek >> Date: Mon Feb 29 10:27:41 2016 >> New Revision: 262232 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=262232&view=rev >> Log: >> Implement new interfaces for code-formatting when applying replacements. >> > > Random request: provide more context in commit logs. =D > > When I randomly end up staring at patches for some reason, its really > useful to know what the background is, or at least a bit of a clue. > > For example, here, why does applying replacements need new interfaces? > > Specifically context for those not 100% actively following along with the > detailed discussion are super useful in the commit log where others end up > reading about changes. > Thanks. Btw, this particular change has been rolled back because it violates layering (that I completely missed). > > >> >> Patch by Eric Liu. >> >> Modified: >> cfe/trunk/include/clang/Tooling/Core/Replacement.h >> cfe/trunk/lib/Tooling/Core/Replacement.cpp >> cfe/trunk/unittests/Tooling/CMakeLists.txt >> cfe/trunk/unittests/Tooling/RefactoringTest.cpp >> >> Modified: cfe/trunk/include/clang/Tooling/Core/Replacement.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=262232&r1=262231&r2=262232&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Tooling/Core/Replacement.h (original) >> +++ cfe/trunk/include/clang/Tooling/Core/Replacement.h Mon Feb 29 >> 10:27:41 2016 >> @@ -30,6 +30,10 @@ namespace clang { >> >> class Rewriter; >> >> +namespace format { >> +struct FormatStyle; >> +} // namespace format >> + >> namespace tooling { >> >> /// \brief A source range independent of the \c SourceManager. >> @@ -220,6 +224,41 @@ bool applyAllReplacements(const std::vec >> /// replacements cannot be applied, this returns an empty \c string. >> std::string applyAllReplacements(StringRef Code, const Replacements >> &Replaces); >> >> +/// \brief Applies all replacements in \p Replaces to \p Code. >> +/// >> +/// This completely ignores the path stored in each replacement. If one >> or more >> +/// replacements cannot be applied, this returns an empty \c string. >> +std::string applyAllReplacements(StringRef Code, >> + const std::vector<Replacements> >> &Replaces); >> + >> +/// \brief Calculate the ranges in a single file that are affected by the >> +/// Replacements. >> +/// >> +/// \pre Replacements must be for the same file. >> +std::vector<tooling::Range> >> +calculateChangedRangesInFile(const tooling::Replacements &Replaces); >> + >> +/// \brief Return replacements that are merged from orginal replacements >> +/// and the replacements for formatting the code after applying the >> orginal >> +/// replacements. >> +tooling::Replacements formatReplacements(StringRef Code, >> + const tooling::Replacements >> &Replaces, >> + const format::FormatStyle >> &Style); >> + >> +/// \brief In addition to applying replacements as in >> `applyAllReplacements`, >> +/// this function also reformats the changed code after applying >> replacements. >> +/// >> +/// \pre Replacements must be for the same file and conflict-free. >> +/// >> +/// Replacement applications happen independently of the success of >> +/// other applications. >> +/// >> +/// \returns the changed code if all replacements apply and code is >> fixed. >> +/// empty string otherwise. >> +std::string applyAllReplacementsAndFormat(StringRef Code, >> + const Replacements &Replaces, >> + const format::FormatStyle >> &Style); >> + >> /// \brief Merges two sets of replacements with the second set referring >> to the >> /// code after applying the first set. Within both 'First' and 'Second', >> /// replacements must not overlap. >> >> Modified: cfe/trunk/lib/Tooling/Core/Replacement.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=262232&r1=262231&r2=262232&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Tooling/Core/Replacement.cpp (original) >> +++ cfe/trunk/lib/Tooling/Core/Replacement.cpp Mon Feb 29 10:27:41 2016 >> @@ -11,17 +11,20 @@ >> // >> >> >> //===----------------------------------------------------------------------===// >> >> +#include "clang/Tooling/Core/Replacement.h" >> + >> #include "clang/Basic/Diagnostic.h" >> #include "clang/Basic/DiagnosticIDs.h" >> #include "clang/Basic/DiagnosticOptions.h" >> #include "clang/Basic/FileManager.h" >> #include "clang/Basic/SourceManager.h" >> +#include "clang/Format/Format.h" >> #include "clang/Lex/Lexer.h" >> #include "clang/Rewrite/Core/Rewriter.h" >> -#include "clang/Tooling/Core/Replacement.h" >> #include "llvm/Support/FileSystem.h" >> #include "llvm/Support/Path.h" >> #include "llvm/Support/raw_os_ostream.h" >> +#include <iterator> >> >> namespace clang { >> namespace tooling { >> @@ -281,6 +284,44 @@ std::string applyAllReplacements(StringR >> return Result; >> } >> >> +tooling::Replacements formatReplacements(StringRef Code, >> + const tooling::Replacements >> &Replaces, >> + const format::FormatStyle >> &Style) { >> + if (Replaces.empty()) return Replacements(); >> + >> + std::string NewCode = applyAllReplacements(Code, Replaces); >> + std::vector<tooling::Range> ChangedRanges = >> + calculateChangedRangesInFile(Replaces); >> + StringRef FileName = Replaces.begin()->getFilePath(); >> + tooling::Replacements FormatReplaces = >> + format::reformat(Style, NewCode, ChangedRanges, FileName); >> + >> + tooling::Replacements MergedReplacements = >> + mergeReplacements(Replaces, FormatReplaces); >> + return MergedReplacements; >> +} >> + >> +std::vector<Range> calculateChangedRangesInFile(const Replacements >> &Replaces) { >> + std::vector<Range> ChangedRanges; >> + int Shift = 0; >> + for (const tooling::Replacement &R : Replaces) { >> + unsigned Offset = R.getOffset() + Shift; >> + unsigned Length = R.getReplacementText().size(); >> + Shift += Length - R.getLength(); >> + ChangedRanges.push_back(tooling::Range(Offset, Length)); >> + } >> + return ChangedRanges; >> +} >> + >> +std::string applyAllReplacementsAndFormat(StringRef Code, >> + const Replacements &Replaces, >> + const format::FormatStyle >> &Style) { >> + Replacements NewReplacements = formatReplacements(Code, Replaces, >> Style); >> + if (NewReplacements.empty()) >> + return Code; // Exit early to avoid overhead in >> `applyAllReplacements`. >> + return applyAllReplacements(Code, NewReplacements); >> +} >> + >> namespace { >> // Represents a merged replacement, i.e. a replacement consisting of >> multiple >> // overlapping replacements from 'First' and 'Second' in >> mergeReplacements. >> @@ -314,7 +355,7 @@ public: >> >> // Merges the next element 'R' into this merged element. As we always >> merge >> // from 'First' into 'Second' or vice versa, the MergedReplacement >> knows what >> - // set the next element is coming from. >> + // set the next element is coming from. >> void merge(const Replacement &R) { >> if (MergeSecond) { >> unsigned REnd = R.getOffset() + Delta + R.getLength(); >> >> Modified: cfe/trunk/unittests/Tooling/CMakeLists.txt >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/CMakeLists.txt?rev=262232&r1=262231&r2=262232&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/Tooling/CMakeLists.txt (original) >> +++ cfe/trunk/unittests/Tooling/CMakeLists.txt Mon Feb 29 10:27:41 2016 >> @@ -24,6 +24,7 @@ target_link_libraries(ToolingTests >> clangAST >> clangASTMatchers >> clangBasic >> + clangFormat >> clangFrontend >> clangLex >> clangRewrite >> >> Modified: cfe/trunk/unittests/Tooling/RefactoringTest.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=262232&r1=262231&r2=262232&view=diff >> >> ============================================================================== >> --- cfe/trunk/unittests/Tooling/RefactoringTest.cpp (original) >> +++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp Mon Feb 29 10:27:41 >> 2016 >> @@ -18,6 +18,7 @@ >> #include "clang/Basic/FileManager.h" >> #include "clang/Basic/LangOptions.h" >> #include "clang/Basic/SourceManager.h" >> +#include "clang/Format/Format.h" >> #include "clang/Frontend/CompilerInstance.h" >> #include "clang/Frontend/FrontendAction.h" >> #include "clang/Frontend/TextDiagnosticPrinter.h" >> @@ -166,6 +167,35 @@ TEST_F(ReplacementTest, ApplyAllFailsIfO >> EXPECT_EQ("z", Context.getRewrittenText(IDz)); >> } >> >> +TEST_F(ReplacementTest, FormatCodeAfterReplacements) { >> + // Column limit is 20. >> + std::string Code = "Type *a =\n" >> + " new Type();\n" >> + "g(iiiii, 0, jjjjj,\n" >> + " 0, kkkkk, 0, mm);\n" >> + "int bad = format ;"; >> + std::string Expected = "auto a = new Type();\n" >> + "g(iiiii, nullptr,\n" >> + " jjjjj, nullptr,\n" >> + " kkkkk, nullptr,\n" >> + " mm);\n" >> + "int bad = format ;"; >> + FileID ID = Context.createInMemoryFile("format.cpp", Code); >> + Replacements Replaces; >> + Replaces.insert( >> + Replacement(Context.Sources, Context.getLocation(ID, 1, 1), 6, >> "auto ")); >> + Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, >> 3, 10), >> + 1, "nullptr")); >> + Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, >> 4, 3), 1, >> + "nullptr")); >> + Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, >> 4, 13), >> + 1, "nullptr")); >> + >> + format::FormatStyle Style = format::getLLVMStyle(); >> + Style.ColumnLimit = 20; // Set column limit to 20 to increase >> readibility. >> + EXPECT_EQ(Expected, applyAllReplacementsAndFormat(Code, Replaces, >> Style)); >> +} >> + >> TEST(ShiftedCodePositionTest, FindsNewCodePosition) { >> Replacements Replaces; >> Replaces.insert(Replacement("", 0, 1, "")); >> @@ -418,6 +448,25 @@ TEST(Range, contains) { >> EXPECT_FALSE(Range(0, 10).contains(Range(0, 11))); >> } >> >> +TEST(Range, CalculateRangesOfReplacements) { >> + // Before: aaaabbbbbbz >> + // After : bbbbbbzzzzzzoooooooooooooooo >> + Replacements Replaces; >> + Replaces.insert(Replacement("foo", 0, 4, "")); >> + Replaces.insert(Replacement("foo", 10, 1, "zzzzzz")); >> + Replaces.insert(Replacement("foo", 11, 0, "oooooooooooooooo")); >> + >> + std::vector<Range> Ranges = calculateChangedRangesInFile(Replaces); >> + >> + EXPECT_EQ(3ul, Ranges.size()); >> + EXPECT_TRUE(Ranges[0].getOffset() == 0); >> + EXPECT_TRUE(Ranges[0].getLength() == 0); >> + EXPECT_TRUE(Ranges[1].getOffset() == 6); >> + EXPECT_TRUE(Ranges[1].getLength() == 6); >> + EXPECT_TRUE(Ranges[2].getOffset() == 12); >> + EXPECT_TRUE(Ranges[2].getLength() == 16); >> +} >> + >> TEST(DeduplicateTest, removesDuplicates) { >> std::vector<Replacement> Input; >> Input.push_back(Replacement("fileA", 50, 0, " foo ")); >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
