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. > > 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
