On Tue, Mar 1, 2016 at 9:30 PM, Manuel Klimek <kli...@google.com> wrote:
> > > 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). > How is that relevant? Removing a trivial function to keep the interface slim seems good either way. We can still discuss this for more complex functions. > > >>> >>>>> >>>>>> >>>>>> 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
