clang-format: better detection for multiplication operator

[Kapfhammer]

inline

On Fri, Apr 04, 2014 at 08:06:00AM +0200, Daniel Jasper wrote:
> I think this makes more sense for now. If we stumble upon bugs requiring
> the fix for the other operators, we still have your other patch.
> 
> 
> On Thu, Apr 3, 2014 at 10:58 PM, Johannes Kapfhammer <jkapfham at ethz.ch>wrote:
> 
> > On Sun, Mar 30, 2014 at 07:45:51PM +0200, Daniel Jasper wrote:
> > > All in all, I think a much easier solution for the mentioned bug would be
> > > to treat "<<" similar to how we treat assignments, i.e. if we find a <<,
> > we
> > > assume the RHS to be an expression (that's the very first "if" in
> > > determineTokenType()).
> > >
> > Not sure if that's the right place.  The "if" branch you are talking
> > about will set all preceding stars (on the same level) to
> > TT_PointerOrReference:
> >
> > Before: cout << a *a << b *b << c *c;
> > After:  cout << a *a << b *b << c * b;
> > My Fix: cout << a * a << b * b << c * c;
> >
> > So we need the special care for operator<< at least in a new
> > "else if".  I've added a "light" version of this patch below, note
> > that it only handles operator<< and doesn't work with other operators.
> > Doing it the way I did in the last mail has the additional benefit
> > that it works for all operators and not just "<<" (in particular
> > operator+ is hard to get otherwise).  But we probably won't need this.
> >
> > --
> >
> > From 16d81e7e3a0dedd5d51f963a134b26eb9c63b777 Mon Sep 17 00:00:00 2001
> > From: Kapfhammer <kapf at student.ethz.ch>
> > Date: Sun, 30 Mar 2014 12:04:17 +0200
> > Subject: [PATCH] Use binary operators as an indicator of for an expression
> > and
> >  extend the test suite.
> >
> > This solves the issue where the star was too often interpreted as a
> > pointer, e.g "cout<<a*b;" was formatted to "cout << a *b;" instead of
> > "cout << a * b;".  By marking statements more often an expression, the
> > function determineStarAmpUsage() is more reliable.
> >
> > The test suite was extended.
> > ---
> >  lib/Format/TokenAnnotator.cpp   |  5 +++++
> >  unittests/Format/FormatTest.cpp | 15 +++++++++++++++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/lib/Format/TokenAnnotator.cpp b/lib/Format/TokenAnnotator.cpp
> > index 0034235..7fbbdae 100644
> > --- a/lib/Format/TokenAnnotator.cpp
> > +++ b/lib/Format/TokenAnnotator.cpp
> > @@ -681,6 +681,11 @@ private:
> >            Previous->Type = TT_PointerOrReference;
> >          }
> >        }
> > +    } else if (Current.is(tok::lessless) &&
> > +               !Line.First->isOneOf(tok::kw_template, tok::kw_using) &&
> >
> 
> How does this change the behavior in any way?
It was relevant with the old patch.  Now it's only different in a few
special cases:

With it: template<int i = x << a *b>
Without: template<int i = x << a * b>

So I think we don't need it.

> 
> 
> > +               (!Current.Previous ||
> > +                Current.Previous->isNot(tok::kw_operator))) {
> >
> 
> I wonder whether we can just use:
> 
> } else if (Current.is(tok::lessless) && !Line.MustBeDeclaration) { ..
> 
I can't tell for sure if it does the same, but it seems so.

> 
> 
> > +      Contexts.back().IsExpression = true;
> >      } else if (Current.isOneOf(tok::kw_return, tok::kw_throw)) {
> >        Contexts.back().IsExpression = true;
> >      } else if (Current.is(tok::l_paren) && !Line.MustBeDeclaration &&
> > diff --git a/unittests/Format/FormatTest.cpp
> > b/unittests/Format/FormatTest.cpp
> > index 5395fd9..7478a23 100644
> > --- a/unittests/Format/FormatTest.cpp
> > +++ b/unittests/Format/FormatTest.cpp
> > @@ -4625,6 +4625,21 @@ TEST_F(FormatTest, UnderstandsRvalueReferences) {
> >    verifyGoogleFormat("#define WHILE(a, b, c) while (a && (b == c))");
> >  }
> >
> > +TEST_F(FormatTest, FormatsMultiplicationOperator) {
> > +  verifyFormat("operator*(type *a)");
> > +  verifyFormat("operator<<(type *a)");
> > +  verifyFormat("{ cout << (a * b); }");
> > +  verifyFormat("{ cout << a * b; }");
> > +  verifyFormat("{ cout << a * b << c * d; }");
> > +  verifyFormat("type (*f)(type *a)");
> > +  verifyFormat("type (&f)(type *a)");
> > +  verifyFormat("void f(type *a)");
> > +  verifyFormat("using f = void (*)(a *b)");
> > +  verifyFormat("template <typename T = void (*)(a *b)");
> > +  verifyFormat("using f = void (c::*)(a *b)");
> > +  verifyFormat("template <typename T = void (c::*)(a *b)>");
> >
> 
> There are many tests here that don't contain a "<<". What are they testing?
> 
They are not needed anymore.  I thought they don't hurt, but you can
remove them if you want to.

> +}
> >
> +
> >  TEST_F(FormatTest, FormatsBinaryOperatorsPrecedingEquals) {
> >    verifyFormat("void f() {\n"
> >                 "  x[aaaaaaaaa -\n"
> > --
> > 1.9.1
> >
> >