r262232 - Implement new interfaces for code-formatting when applying replacements.
Manuel Klimek via cfe-commits
cfe-commits at lists.llvm.org
Tue Mar 1 21:58:06 PST 2016
On Wed, Mar 2, 2016 at 6:42 AM Daniel Jasper <djasper at google.com> wrote:
> On Tue, Mar 1, 2016 at 9:30 PM, Manuel Klimek <klimek at google.com> wrote:
>
>>
>>
>> On Wed, Mar 2, 2016 at 6:20 AM Daniel Jasper <djasper at google.com> wrote:
>>
>>> On Tue, Mar 1, 2016 at 9:12 PM, Daniel Jasper <djasper at google.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> 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.
>>>>
>>>
>>> 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 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/20160302/086470b5/attachment-0001.html>
More information about the cfe-commits
mailing list