On Wed, Mar 2, 2016 at 6:42 AM Daniel Jasper <djas...@google.com> wrote:
> 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. > Sure. I think we're also discussion this in the wrong thread :) > > >> >> >>>> >>>>>> >>>>>>> >>>>>>> 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
