<div dir="ltr">I think, we cannot submit this without actual tests. My main problem is that I think line removals leading to two consecutive empty lines are a rare corner case. I am not saying, it is not worth fixing, but it is much more important that the common case still works fine. Specifically, if we delete a line between other lines, none of the other lines should be touched. I believe we can make this work, but it needs tests.<div>
<br></div><div>Other questions/remarks:</div><div>- Could you generally use Phabricator? We do all clang-format related code reviews there and once you are used to it, it gets hard to live without ;-).</div><div>- Can you describe the behavior of -lines that the user can expect (preferably in the usage instructions)?</div>
<div>- I don't think it is a good idea to allow from-line/to-line in any order. What we need is a way to specify that we want to format a zero-length region at the start of a specific line. I am fine with making "-lines=5:4" a special syntax for that, but I would not generalize. That just gets too confusing and I don't think anyone will really need it.</div>
<div><br></div><div><div> + SourceLocation End = ToLine > 0</div><div> + ? Sources.translateLineCol(ID, ToLine, UINT_MAX)</div><div> + : Sources.getLocForStartOfFile(ID);</div>
</div><div><br></div><div>I'd just make this:</div><div><br></div><div> ToLine = std::max(0u, ToLine);</div><div> SourceLocation End = Sources.translateLineCol(ID, ToLine, UINT_MAX);</div><div><br></div><div>The reason I think we need tests is that the following seems wrong:</div>
<div><br></div><div><div> if line_count == 0:</div><div> - continue</div><div> + start_line += 1</div><div> end_line = start_line + line_count - 1;</div></div><div><br></div><div>I don't really understand why start_line has to be incremented. That probably means that it needs a comment, possibly with an example diff or something.</div>
<div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 21, 2013 at 8:27 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank" class="cremed">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
Hi Daniel,<br>
<br>
This patch is the continuation of r191820. The purpose is to
properly reformat whitespace surrounding line removals in
clang-format-diff.py without reformatting unrelated code as was
happening before.<br>
<br>
For example, the input:<br>
<br>
<pre>diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp</pre>
<pre>index a71d3c0..a75d66f 100644</pre>
<pre>--- a/lib/Sema/SemaDeclAttr.cpp</pre>
<pre>+++ b/lib/Sema/SemaDeclAttr.cpp</pre>
<pre>@@ -1681,10 +1681,10 @@ static void handleAliasAttr(Sema &S, Decl *D, const AttributeList &Attr) {</pre>
<pre> return;</pre>
<pre> }</pre>
<pre> </pre>
<pre>- // FIXME: check if target symbol exists in current file</pre>
<pre> </pre>
<pre> D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,</pre>
<pre> Attr.getAttributeSpellingListIndex()));</pre>
<pre>+ S.Aliases.push_back(D);</pre>
<pre> }</pre>
<pre> </pre>
<pre> static void handleMinSizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {</pre>
<br>
Now yields:<br>
<br>
<pre>--- lib/Sema/SemaDeclAttr.cpp (before formatting)</pre>
<pre>+++ lib/Sema/SemaDeclAttr.cpp (after formatting)</pre>
<pre>@@ -1680,7 +1680,6 @@</pre>
<pre> S.Diag(Attr.getLoc(), diag::err_alias_not_supported_on_darwin);</pre>
<pre> return;</pre>
<pre> }</pre>
<pre>-</pre>
<pre> </pre>
<pre> D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,</pre>
<pre> Attr.getAttributeSpellingListIndex()));</pre>
<br>
Even though we're not yet testing the scripts, I'm aware this needs
a test added to test/Format/line-ranges.cpp. Unfortunately I'm
struggling to write one and found the existing tests hard to read.
Could you give me a hand with that?<br>
<br>
I wonder if we could move towards multiple input files for a single
test -- just trying to wrap my head around line-ranges.cpp,
FileCheck doesn't feel like the right tool for this. I wonder if GNU
diffutils has a better format for tests that we could adopt to start
testing clang-format-diff.py.<span class="HOEnZb"><font color="#888888"><br>
<br>
Alp.<br>
<br>
<pre cols="72">--
<a href="http://www.nuanti.com" target="_blank" class="cremed">http://www.nuanti.com</a>
the browser experts
</pre>
</font></span></div>
</blockquote></div><br></div></div>