[PATCH] clang-format: Fix whitespace surrounding diff removals
Daniel Jasper
djasper at google.com
Tue Oct 22 06:28:28 PDT 2013
> 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.
>
Yes, I know, but I am also at a loss on what might be the best way.
I guess I would do the following:
- Create the input you want to execute formatting on in source.cpp.
- Manually author source.diff faking changes to source.cpp.
- Turn source.cpp into a FileCheck-based test using clang-format-diff.py
and source.diff
I can take a stab at this, but I won't get to it before tomorrow.
> > 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.
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.
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.
> 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.
>
Right, but if something doesn't work as expected, deletions might
deteriorate. Thus, I think this is a perfect time to add tests.
> 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 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) ..
Also, clang-format will stop at the next unwrapped line (e.g. statement).
So, it will fail to correct:
if (a)
if (b)
c();
when just "if (a)" is deleted.
But I guess all of those will be corner cases, so I am happy giving this a
try.
> 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.
>
Hotmail? You need a GitHub account ..
> - 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.
>
I don't understand what a 3-way merge has to do with this ..
>
> > + 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.
>
I see.
>
> > 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.
>
Well, giving some derivation of that quote in a comment would also help.
In the meantime I think the patch should be useful to people using
> clang-format-diff.py to work on LLVM git/svn.
>
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.
Thanks for working on this (I always had to push it because of other stuff
:-/),
Daniel
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131022/9586933a/attachment.html>
More information about the cfe-commits
mailing list