[PATCH] clang-format: Fix whitespace surrounding diff removals

[Alp Toker]

On 22/10/2013 11:53, Daniel Jasper wrote:
> I think, we cannot submit this without actual tests.

Agreed, I haven't figured out the best way to test this yet and asked
for help with that.

> My main problem is that I think line removals leading to two
> consecutive empty lines are a rare corner case.

Line removals are the main use case for a dead-code removal tool, but
also happen often in day-to-day coding :-) Going by previous discussions
I think we're using clang-format in very different ways which is great.

If you look at cfe-commits, you can tell who's using
clang-format-diff.py the same way as I am: Before r191820, many patches
were over-formatting surrounding code which was annoying.

We fixed that in r191820, but ended up skipping removed hunks as a
side-effect. You can see recent cfe-commits submissions that are leaving
behind contiguous blank lines now, as in the example from my original mail.

> I am not saying, it is not worth fixing, but it is much more important
> that the common case still works fine.

This patch shouldn't affect formatting of whole files, line-addition or
positive ranges at all. It just restores the pre-r191820 behaviour more
conservatively.

> Specifically, if we delete a line between other lines, none of the
> other lines should be touched.

Almost true. In the following example, if we remove the first line, we
do in fact want the second line to be deindented:

if (true)
  body();

This is taken care of.

> I believe we can make this work, but it needs tests.
>
> Other questions/remarks:
> - 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 ;-).

Sure, once it's live on llvm.org and hooked up to the accounts. I'm not
getting a Hotmail/Gmail account just to sign in to that.

> - Can you describe the behavior of -lines that the user can expect
> (preferably in the usage instructions)?
> - 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.

We could restrict this to one line boundary for now, but I have a
feeling the inverse syntax will prove useful for resolving whitespace
conflicts such as in 3-way-merge. Will investigate that further.

>
>   +      SourceLocation End = ToLine > 0
>   +                               ? Sources.translateLineCol(ID,
> ToLine, UINT_MAX)
>   +                               : Sources.getLocForStartOfFile(ID);
>
> I'd just make this:
>
>   ToLine = std::max(0u, ToLine);
>   SourceLocation End = Sources.translateLineCol(ID, ToLine, UINT_MAX);

That would crash.

>
> The reason I think we need tests is that the following seems wrong:
>
>          if line_count == 0:
>   -        continue
>   +        start_line += 1
>          end_line = start_line + line_count - 1;
>
> 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.

"An empty hunk is considered to start at the line that follows the hunk.
An empty hunk is considered to end at the line that precedes the hunk."

Agree on needing examples -- it's time for diff tests.

In the meantime I think the patch should be useful to people using
clang-format-diff.py to work on LLVM git/svn.

Alp.


>
> On Mon, Oct 21, 2013 at 8:27 PM, Alp Toker <alp at nuanti.com
> <mailto:alp at nuanti.com>> wrote:
>
>     Hi Daniel,
>
>     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.
>
>     For example, the input:
>
>     diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
>
>     index a71d3c0..a75d66f 100644
>
>     --- a/lib/Sema/SemaDeclAttr.cpp
>
>     +++ b/lib/Sema/SemaDeclAttr.cpp
>
>     @@ -1681,10 +1681,10 @@ static void handleAliasAttr(Sema &S, Decl *D, const AttributeList &Attr) {
>
>          return;
>
>        }
>
>      
>
>     -  // FIXME: check if target symbol exists in current file
>
>      
>
>        D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,
>
>                                               Attr.getAttributeSpellingListIndex()));
>
>     +  S.Aliases.push_back(D);
>
>      }
>
>      
>
>      static void handleMinSizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
>
>
>     Now yields:
>
>     --- lib/Sema/SemaDeclAttr.cpp    (before formatting)
>
>     +++ lib/Sema/SemaDeclAttr.cpp    (after formatting)
>
>     @@ -1680,7 +1680,6 @@
>
>          S.Diag(Attr.getLoc(), diag::err_alias_not_supported_on_darwin);
>
>          return;
>
>        }
>
>     -
>
>      
>
>        D->addAttr(::new (S.Context) AliasAttr(Attr.getRange(), S.Context, Str,
>
>                                               Attr.getAttributeSpellingListIndex()));
>
>
>     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?
>
>     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.
>
>     Alp.
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>

-- 
http://www.nuanti.com
the browser experts