<div dir="ltr"><br><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>
On 22/10/2013 11:53, Daniel Jasper wrote:<br>
> I think, we cannot submit this without actual tests.<br>
<br>
</div>Agreed, I haven't figured out the best way to test this yet and asked<br>
for help with that.<br></blockquote><div><br></div><div>Yes, I know, but I am also at a loss on what might be the best way.</div><div><br></div><div>I guess I would do the following:<br>- Create the input you want to execute formatting on in source.cpp.</div>
<div>- Manually author source.diff faking changes to source.cpp.</div><div>- Turn source.cpp into a FileCheck-based test using clang-format-diff.py and source.diff</div><div><br></div><div>I can take a stab at this, but I won't get to it before tomorrow.</div>
<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">
<div>
> My main problem is that I think line removals leading to two<br>
> consecutive empty lines are a rare corner case.<br>
<br>
</div>Line removals are the main use case for a dead-code removal tool, but<br>
also happen often in day-to-day coding :-) Going by previous discussions<br>
I think we're using clang-format in very different ways which is great. </blockquote><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">


If you look at cfe-commits, you can tell who's using<br>
clang-format-diff.py the same way as I am: Before r191820, many patches<br>
were over-formatting surrounding code which was annoying.<br>
<br>
We fixed that in r191820, but ended up skipping removed hunks as a<br>
side-effect. You can see recent cfe-commits submissions that are leaving<br>
behind contiguous blank lines now, as in the example from my original mail.</blockquote><div><br class="">Well, I still think the chance of somebody deleting all the code between two empty lines and not thinking to delete one of the empty lines is reasonably slim.</div>
<div><br></div><div>However, again, I am not saying we shouldn't do it, but I really want to make sure that we don't touch the 99% use case.</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>
> I am not saying, it is not worth fixing, but it is much more important<br>
> that the common case still works fine.<br>
<br>
</div>This patch shouldn't affect formatting of whole files, line-addition or<br>
positive ranges at all. It just restores the pre-r191820 behaviour more<br>
conservatively.<br></blockquote><div><br></div><div>Right, but if something doesn't work as expected, deletions might deteriorate. Thus, I think this is a perfect time to add tests.</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>
> Specifically, if we delete a line between other lines, none of the<br>
> other lines should be touched.<br>
<br>
</div>Almost true. In the following example, if we remove the first line, we<br>
do in fact want the second line to be deindented:<br>
<br>
if (true)<br>
  body();<br>
<br>
This is taken care of.<br></blockquote><div><br></div><div>I am not absolutely convinced that this is behavior we'd want. Suppose somebody does not agree with clang-format's choice of indentation for the next line (and that line is unrelated) .. </div>
<div><br></div><div>Also, clang-format will stop at the next unwrapped line (e.g. statement). So, it will fail to correct:</div><div><br></div><div>if (a)</div><div>  if (b)</div><div>    c();</div><div><br></div><div>when just "if (a)" is deleted.</div>
<div><br></div><div>But I guess all of those will be corner cases, so I am happy giving this a try.</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>
> I believe we can make this work, but it needs tests.<br>
><br>
> Other questions/remarks:<br>
> - Could you generally use Phabricator? We do all clang-format related<br>
> code reviews there and once you are used to it, it gets hard to live<br>
> without ;-).<br>
<br>
</div>Sure, once it's live on <a href="http://llvm.org" target="_blank" class="cremed">llvm.org</a> and hooked up to the accounts. I'm not<br>
getting a Hotmail/Gmail account just to sign in to that.<br></blockquote><div><br></div><div>Hotmail? You need a GitHub account ..</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>
> - Can you describe the behavior of -lines that the user can expect<br>
> (preferably in the usage instructions)?<br>
> - I don't think it is a good idea to allow from-line/to-line in any<br>
> order. What we need is a way to specify that we want to format a<br>
> zero-length region at the start of a specific line. I am fine with<br>
> making "-lines=5:4" a special syntax for that, but I would not<br>
> generalize. That just gets too confusing and I don't think anyone will<br>
> really need it.<br>
<br>
</div>We could restrict this to one line boundary for now, but I have a<br>
feeling the inverse syntax will prove useful for resolving whitespace<br>
conflicts such as in 3-way-merge. Will investigate that further.<br></blockquote><div><br></div><div>I don't understand what a 3-way merge has to do with this .. </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>
><br>
>   +      SourceLocation End = ToLine > 0<br>
>   +                               ? Sources.translateLineCol(ID,<br>
> ToLine, UINT_MAX)<br>
>   +                               : Sources.getLocForStartOfFile(ID);<br>
><br>
> I'd just make this:<br>
><br>
>   ToLine = std::max(0u, ToLine);<br>
>   SourceLocation End = Sources.translateLineCol(ID, ToLine, UINT_MAX);<br>
<br>
</div>That would crash.<br></blockquote><div><br></div><div>I see. </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>
><br>
> The reason I think we need tests is that the following seems wrong:<br>
><br>
>          if line_count == 0:<br>
>   -        continue<br>
>   +        start_line += 1<br>
>          end_line = start_line + line_count - 1;<br>
><br>
> I don't really understand why start_line has to be incremented. That<br>
> probably means that it needs a comment, possibly with an example diff<br>
> or something.<br>
<br>
</div>"An empty hunk is considered to start at the line that follows the hunk.<br>
An empty hunk is considered to end at the line that precedes the hunk."<br>
<br>
Agree on needing examples -- it's time for diff tests.<br></blockquote><div><br></div><div>Well, giving some derivation of that quote in a comment would also help.</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">

In the meantime I think the patch should be useful to people using<br>
clang-format-diff.py to work on LLVM git/svn.<br></blockquote><div><br></div><div>Please add the comments I requested and the description of the -lines usage and then this should be fine to go in. I'd prefer tests at this stage, but if you don't get to it, I will try to add them as a follow-up.</div>
<div><br></div><div>Thanks for working on this (I always had to push it because of other stuff :-/),</div><div>Daniel</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">
<span><font color="#888888">
Alp.<br>
</font></span><div><br>
<br>
><br>
> On Mon, Oct 21, 2013 at 8:27 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank" class="cremed">alp@nuanti.com</a><br>
</div><div><div>> <mailto:<a href="mailto:alp@nuanti.com" target="_blank" class="cremed">alp@nuanti.com</a>>> wrote:<br>
><br>
>     Hi Daniel,<br>
><br>
>     This patch is the continuation of r191820. The purpose is to<br>
>     properly reformat whitespace surrounding line removals in<br>
>     clang-format-diff.py without reformatting unrelated code as was<br>
>     happening before.<br>
><br>
>     For example, the input:<br>
><br>
>     diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp<br>
><br>
>     index a71d3c0..a75d66f 100644<br>
><br>
>     --- a/lib/Sema/SemaDeclAttr.cpp<br>
><br>
>     +++ b/lib/Sema/SemaDeclAttr.cpp<br>
><br>
>     @@ -1681,10 +1681,10 @@ static void handleAliasAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
><br>
>          return;<br>
><br>
>        }<br>
><br>
><br>
><br>
>     -  // FIXME: check if target symbol exists in current file<br>
><br>
><br>
><br>
>        D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,<br>
><br>
>                                               Attr.getAttributeSpellingListIndex()));<br>
><br>
>     +  S.Aliases.push_back(D);<br>
><br>
>      }<br>
><br>
><br>
><br>
>      static void handleMinSizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {<br>
><br>
><br>
>     Now yields:<br>
><br>
>     --- lib/Sema/SemaDeclAttr.cpp    (before formatting)<br>
><br>
>     +++ lib/Sema/SemaDeclAttr.cpp    (after formatting)<br>
><br>
>     @@ -1680,7 +1680,6 @@<br>
><br>
>          S.Diag(Attr.getLoc(), diag::err_alias_not_supported_on_darwin);<br>
><br>
>          return;<br>
><br>
>        }<br>
><br>
>     -<br>
><br>
><br>
><br>
>        D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,<br>
><br>
>                                               Attr.getAttributeSpellingListIndex()));<br>
><br>
><br>
>     Even though we're not yet testing the scripts, I'm aware this<br>
>     needs a test added to test/Format/line-ranges.cpp. Unfortunately<br>
>     I'm struggling to write one and found the existing tests hard to<br>
>     read. Could you give me a hand with that?<br>
><br>
>     I wonder if we could move towards multiple input files for a<br>
>     single test -- just trying to wrap my head around line-ranges.cpp,<br>
>     FileCheck doesn't feel like the right tool for this. I wonder if<br>
>     GNU diffutils has a better format for tests that we could adopt to<br>
>     start testing clang-format-diff.py.<br>
><br>
>     Alp.<br>
><br>
>     --<br>
>     <a href="http://www.nuanti.com" target="_blank" class="cremed">http://www.nuanti.com</a><br>
>     the browser experts<br>
><br>
><br>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank" class="cremed">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</div></div></blockquote></div><br></div></div>