r262232 - Implement new interfaces for code-formatting when applying replacements.
Daniel Jasper via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 1 21:12:17 PST 2016
On Mon, Feb 29, 2016 at 8:49 AM, Manuel Klimek via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> On Mon, Feb 29, 2016 at 5:39 PM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
>> On Mon, Feb 29, 2016 at 11:32 AM Manuel Klimek via cfe-commits <
>> cfe-commits at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160301/d347dd3c/attachment-0001.html>
More information about the cfe-commits
mailing list