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

[Daniel Jasper]

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.

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 ;-).
- 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.

  +      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);

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.

On Mon, Oct 21, 2013 at 8:27 PM, Alp Toker <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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131022/860b0700/attachment.html>