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