On Mon, Feb 29, 2016 at 8:49 AM, Manuel Klimek via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> 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). > The rollback similarly lacks a proper commit comment. Why was it rolled back? I actually think it should not violate layering. libTooling can depend on libFormat which in turn only depends on libToolingCore. I specifically don't want libFormat to have to depend on libTooling. That was kind of the reason why I separated libTooling into libTooling and libToolingCore in the first place. >> >>> >>> 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 > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
