<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Mar 2, 2016 at 6:20 AM Daniel Jasper <<a href="mailto:djasper@google.com">djasper@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Mar 1, 2016 at 9:12 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Mon, Feb 29, 2016 at 8:49 AM, Manuel Klimek via cfe-commits <span dir="ltr"><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><span><div dir="ltr">On Mon, Feb 29, 2016 at 5:39 PM Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, Feb 29, 2016 at 11:32 AM Manuel Klimek via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: klimek<br>
Date: Mon Feb 29 10:27:41 2016<br>
New Revision: 262232<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=262232&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=262232&view=rev</a><br>
Log:<br>
Implement new interfaces for code-formatting when applying replacements.<br></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>Random request: provide more context in commit logs. =D</div><div><br></div><div>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.</div><div><br></div><div>For example, here, why does applying replacements need new interfaces?</div><div><br></div><div>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.</div></div></div></blockquote><div><br></div></span><div>Thanks. Btw, this particular change has been rolled back because it violates layering (that I completely missed).</div></div></div></blockquote><div><br></div></span><div>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.</div></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Ah, never mind, this isn't actually about libTooling, just about libToolingCore. So that is fine.</div><div><br></div><div>As for the code itself:</div><div>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()).<br></div></div></div></div></blockquote><div><br></div><div>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).<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Patch by Eric Liu.<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Tooling/Core/Replacement.h<br>
cfe/trunk/lib/Tooling/Core/Replacement.cpp<br>
cfe/trunk/unittests/Tooling/CMakeLists.txt<br>
cfe/trunk/unittests/Tooling/RefactoringTest.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Tooling/Core/Replacement.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=262232&r1=262231&r2=262232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Tooling/Core/Replacement.h?rev=262232&r1=262231&r2=262232&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Tooling/Core/Replacement.h (original)<br>
+++ cfe/trunk/include/clang/Tooling/Core/Replacement.h Mon Feb 29 10:27:41 2016<br>
@@ -30,6 +30,10 @@ namespace clang {<br>
<br>
class Rewriter;<br>
<br>
+namespace format {<br>
+struct FormatStyle;<br>
+} // namespace format<br>
+<br>
namespace tooling {<br>
<br>
/// \brief A source range independent of the \c SourceManager.<br>
@@ -220,6 +224,41 @@ bool applyAllReplacements(const std::vec<br>
/// replacements cannot be applied, this returns an empty \c string.<br>
std::string applyAllReplacements(StringRef Code, const Replacements &Replaces);<br>
<br>
+/// \brief Applies all replacements in \p Replaces to \p Code.<br>
+///<br>
+/// This completely ignores the path stored in each replacement. If one or more<br>
+/// replacements cannot be applied, this returns an empty \c string.<br>
+std::string applyAllReplacements(StringRef Code,<br>
+ const std::vector<Replacements> &Replaces);<br>
+<br>
+/// \brief Calculate the ranges in a single file that are affected by the<br>
+/// Replacements.<br>
+///<br>
+/// \pre Replacements must be for the same file.<br>
+std::vector<tooling::Range><br>
+calculateChangedRangesInFile(const tooling::Replacements &Replaces);<br>
+<br>
+/// \brief Return replacements that are merged from orginal replacements<br>
+/// and the replacements for formatting the code after applying the orginal<br>
+/// replacements.<br>
+tooling::Replacements formatReplacements(StringRef Code,<br>
+ const tooling::Replacements &Replaces,<br>
+ const format::FormatStyle &Style);<br>
+<br>
+/// \brief In addition to applying replacements as in `applyAllReplacements`,<br>
+/// this function also reformats the changed code after applying replacements.<br>
+///<br>
+/// \pre Replacements must be for the same file and conflict-free.<br>
+///<br>
+/// Replacement applications happen independently of the success of<br>
+/// other applications.<br>
+///<br>
+/// \returns the changed code if all replacements apply and code is fixed.<br>
+/// empty string otherwise.<br>
+std::string applyAllReplacementsAndFormat(StringRef Code,<br>
+ const Replacements &Replaces,<br>
+ const format::FormatStyle &Style);<br>
+<br>
/// \brief Merges two sets of replacements with the second set referring to the<br>
/// code after applying the first set. Within both 'First' and 'Second',<br>
/// replacements must not overlap.<br>
<br>
Modified: cfe/trunk/lib/Tooling/Core/Replacement.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=262232&r1=262231&r2=262232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Tooling/Core/Replacement.cpp?rev=262232&r1=262231&r2=262232&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp (original)<br>
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp Mon Feb 29 10:27:41 2016<br>
@@ -11,17 +11,20 @@<br>
//<br>
//===----------------------------------------------------------------------===//<br>
<br>
+#include "clang/Tooling/Core/Replacement.h"<br>
+<br>
#include "clang/Basic/Diagnostic.h"<br>
#include "clang/Basic/DiagnosticIDs.h"<br>
#include "clang/Basic/DiagnosticOptions.h"<br>
#include "clang/Basic/FileManager.h"<br>
#include "clang/Basic/SourceManager.h"<br>
+#include "clang/Format/Format.h"<br>
#include "clang/Lex/Lexer.h"<br>
#include "clang/Rewrite/Core/Rewriter.h"<br>
-#include "clang/Tooling/Core/Replacement.h"<br>
#include "llvm/Support/FileSystem.h"<br>
#include "llvm/Support/Path.h"<br>
#include "llvm/Support/raw_os_ostream.h"<br>
+#include <iterator><br>
<br>
namespace clang {<br>
namespace tooling {<br>
@@ -281,6 +284,44 @@ std::string applyAllReplacements(StringR<br>
return Result;<br>
}<br>
<br>
+tooling::Replacements formatReplacements(StringRef Code,<br>
+ const tooling::Replacements &Replaces,<br>
+ const format::FormatStyle &Style) {<br>
+ if (Replaces.empty()) return Replacements();<br>
+<br>
+ std::string NewCode = applyAllReplacements(Code, Replaces);<br>
+ std::vector<tooling::Range> ChangedRanges =<br>
+ calculateChangedRangesInFile(Replaces);<br>
+ StringRef FileName = Replaces.begin()->getFilePath();<br>
+ tooling::Replacements FormatReplaces =<br>
+ format::reformat(Style, NewCode, ChangedRanges, FileName);<br>
+<br>
+ tooling::Replacements MergedReplacements =<br>
+ mergeReplacements(Replaces, FormatReplaces);<br>
+ return MergedReplacements;<br>
+}<br>
+<br>
+std::vector<Range> calculateChangedRangesInFile(const Replacements &Replaces) {<br>
+ std::vector<Range> ChangedRanges;<br>
+ int Shift = 0;<br>
+ for (const tooling::Replacement &R : Replaces) {<br>
+ unsigned Offset = R.getOffset() + Shift;<br>
+ unsigned Length = R.getReplacementText().size();<br>
+ Shift += Length - R.getLength();<br>
+ ChangedRanges.push_back(tooling::Range(Offset, Length));<br>
+ }<br>
+ return ChangedRanges;<br>
+}<br>
+<br>
+std::string applyAllReplacementsAndFormat(StringRef Code,<br>
+ const Replacements &Replaces,<br>
+ const format::FormatStyle &Style) {<br>
+ Replacements NewReplacements = formatReplacements(Code, Replaces, Style);<br>
+ if (NewReplacements.empty())<br>
+ return Code; // Exit early to avoid overhead in `applyAllReplacements`.<br>
+ return applyAllReplacements(Code, NewReplacements);<br>
+}<br>
+<br>
namespace {<br>
// Represents a merged replacement, i.e. a replacement consisting of multiple<br>
// overlapping replacements from 'First' and 'Second' in mergeReplacements.<br>
@@ -314,7 +355,7 @@ public:<br>
<br>
// Merges the next element 'R' into this merged element. As we always merge<br>
// from 'First' into 'Second' or vice versa, the MergedReplacement knows what<br>
- // set the next element is coming from.<br>
+ // set the next element is coming from.<br>
void merge(const Replacement &R) {<br>
if (MergeSecond) {<br>
unsigned REnd = R.getOffset() + Delta + R.getLength();<br>
<br>
Modified: cfe/trunk/unittests/Tooling/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/CMakeLists.txt?rev=262232&r1=262231&r2=262232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/CMakeLists.txt?rev=262232&r1=262231&r2=262232&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Tooling/CMakeLists.txt (original)<br>
+++ cfe/trunk/unittests/Tooling/CMakeLists.txt Mon Feb 29 10:27:41 2016<br>
@@ -24,6 +24,7 @@ target_link_libraries(ToolingTests<br>
clangAST<br>
clangASTMatchers<br>
clangBasic<br>
+ clangFormat<br>
clangFrontend<br>
clangLex<br>
clangRewrite<br>
<br>
Modified: cfe/trunk/unittests/Tooling/RefactoringTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=262232&r1=262231&r2=262232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/unittests/Tooling/RefactoringTest.cpp?rev=262232&r1=262231&r2=262232&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/unittests/Tooling/RefactoringTest.cpp (original)<br>
+++ cfe/trunk/unittests/Tooling/RefactoringTest.cpp Mon Feb 29 10:27:41 2016<br>
@@ -18,6 +18,7 @@<br>
#include "clang/Basic/FileManager.h"<br>
#include "clang/Basic/LangOptions.h"<br>
#include "clang/Basic/SourceManager.h"<br>
+#include "clang/Format/Format.h"<br>
#include "clang/Frontend/CompilerInstance.h"<br>
#include "clang/Frontend/FrontendAction.h"<br>
#include "clang/Frontend/TextDiagnosticPrinter.h"<br>
@@ -166,6 +167,35 @@ TEST_F(ReplacementTest, ApplyAllFailsIfO<br>
EXPECT_EQ("z", Context.getRewrittenText(IDz));<br>
}<br>
<br>
+TEST_F(ReplacementTest, FormatCodeAfterReplacements) {<br>
+ // Column limit is 20.<br>
+ std::string Code = "Type *a =\n"<br>
+ " new Type();\n"<br>
+ "g(iiiii, 0, jjjjj,\n"<br>
+ " 0, kkkkk, 0, mm);\n"<br>
+ "int bad = format ;";<br>
+ std::string Expected = "auto a = new Type();\n"<br>
+ "g(iiiii, nullptr,\n"<br>
+ " jjjjj, nullptr,\n"<br>
+ " kkkkk, nullptr,\n"<br>
+ " mm);\n"<br>
+ "int bad = format ;";<br>
+ FileID ID = Context.createInMemoryFile("format.cpp", Code);<br>
+ Replacements Replaces;<br>
+ Replaces.insert(<br>
+ Replacement(Context.Sources, Context.getLocation(ID, 1, 1), 6, "auto "));<br>
+ Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, 3, 10),<br>
+ 1, "nullptr"));<br>
+ Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, 4, 3), 1,<br>
+ "nullptr"));<br>
+ Replaces.insert(Replacement(Context.Sources, Context.getLocation(ID, 4, 13),<br>
+ 1, "nullptr"));<br>
+<br>
+ format::FormatStyle Style = format::getLLVMStyle();<br>
+ Style.ColumnLimit = 20; // Set column limit to 20 to increase readibility.<br>
+ EXPECT_EQ(Expected, applyAllReplacementsAndFormat(Code, Replaces, Style));<br>
+}<br>
+<br>
TEST(ShiftedCodePositionTest, FindsNewCodePosition) {<br>
Replacements Replaces;<br>
Replaces.insert(Replacement("", 0, 1, ""));<br>
@@ -418,6 +448,25 @@ TEST(Range, contains) {<br>
EXPECT_FALSE(Range(0, 10).contains(Range(0, 11)));<br>
}<br>
<br>
+TEST(Range, CalculateRangesOfReplacements) {<br>
+ // Before: aaaabbbbbbz<br>
+ // After : bbbbbbzzzzzzoooooooooooooooo<br>
+ Replacements Replaces;<br>
+ Replaces.insert(Replacement("foo", 0, 4, ""));<br>
+ Replaces.insert(Replacement("foo", 10, 1, "zzzzzz"));<br>
+ Replaces.insert(Replacement("foo", 11, 0, "oooooooooooooooo"));<br>
+<br>
+ std::vector<Range> Ranges = calculateChangedRangesInFile(Replaces);<br>
+<br>
+ EXPECT_EQ(3ul, Ranges.size());<br>
+ EXPECT_TRUE(Ranges[0].getOffset() == 0);<br>
+ EXPECT_TRUE(Ranges[0].getLength() == 0);<br>
+ EXPECT_TRUE(Ranges[1].getOffset() == 6);<br>
+ EXPECT_TRUE(Ranges[1].getLength() == 6);<br>
+ EXPECT_TRUE(Ranges[2].getOffset() == 12);<br>
+ EXPECT_TRUE(Ranges[2].getLength() == 16);<br>
+}<br>
+<br>
TEST(DeduplicateTest, removesDuplicates) {<br>
std::vector<Replacement> Input;<br>
Input.push_back(Replacement("fileA", 50, 0, " foo "));<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div></blockquote></div></div>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div></div></div><br></div></div>
</blockquote></div></div></div></blockquote></div></div>