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:20:16 PST 2016
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()).
>
>
>>>
>>>>
>>>> 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/62d1c998/attachment-0001.html>
More information about the cfe-commits
mailing list