On Wed, Mar 2, 2016 at 6:20 AM Daniel Jasper <djas...@google.com> wrote:
> On Tue, Mar 1, 2016 at 9:12 PM, Daniel Jasper <djas...@google.com> wrote: > >> >> >> 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. >> > > Ah, never mind, this isn't actually about libTooling, just about > libToolingCore. So that is fine. > > As for the code itself: > applyAllReplacementsAndFormat seems like the wrong interface to me as it > does two things. I think we should sink the "if (NewReplacements.empty())" > test into applyAllReplacements, by which point > applyAllReplacementsAndFormat() is just a very slightly shorter form of: > applyAllReplacements(formatReplacements()). > The problem is how to express that on the Rewriter interface (that function is not in yet, as it requires pulling out some other common functions we don't have yet). >> >>>> >>>>> >>>>> 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
