Some heuristics to identify c style casting (PR18937)

[dinesh.d]

Summary

I have added few heuristics to identify c style casting. Only side
effect of this patch is that single identifier wrapped with parentheses
(identifier) are getting treated as cast.

[dinesh.d]

gentle ping

[djasper]

Thank you for working on this!

lib/Format/TokenAnnotator.cpp

757

Please add:

// FIXME: Pull cast detection into its own function.

790

Please use {} for the if, if there are {} for the else.

792

I'd prefer to put this comment on its own line.

And please make all comments here full sentences.

799

I think we need to check that Current.Next is not null. Otherwise, this might crash if a line ends in a right parenthesis.

On the other hand, I don't think "Prev &&" changes the behavior.

How about writing this as:

if (Current.Next && Current.Next->Next) {
  bool NextIsUnary = Current.Next->isUnaryOperator() ||
                     Current.Next->isOneOf(tok::amp, tok::star);
  IsCast = NextIsUnary && Current.Next->Next->isOneOf(
                              tok::identifier, tok::numeric_constant);
}

I think then it is almost self-explanatory and you don't need the comment.

806

What about:

• * (pointers)
• (arrays)
• <> (templates)

?

808

Further up in this function, we are using a for-loop for this, i.e.

for ( ; Prev != Current.MatchingParen; Prev = Prev->Previous) { ..

I think we should be consistent.

[dinesh.d]

updated patch as per comments

lib/Format/TokenAnnotator.cpp

757

added comment

790

updated

799

updated. I am setting IsCast to true and then if it is not identified as cast, it is getting resetted.
"(Prev && .." was there to keep IsCast becomming true as it will not get reset later.

I am not sure if I have to check for Prev or can assume that it will always be non-null here.

806

I did not get it. Do you mean something like *(my_int_ptr)+2 etc. I will look for other
casting example and try to handle them in patch.

If you have anything specific in mind, which should be handled, please suggest.

808

updated

[djasper]

Two nits, otherwise looks good.

lib/Format/TokenAnnotator.cpp

794

nit: Period at the end of a sentence.

806

I mean casts like:

vector<string>* v = (vector<string>*)x;

But nevermind, I think then we don't even get into this branch.

807

nit: I'd prefer {} here..

[dinesh.d]

updated as per comments

[dinesh.d]

lib/Format/TokenAnnotator.cpp

794

updated

807

updated

[dinesh.d]

submitted as r207964.

Thanks for review and support.

lib/Format/TokenAnnotator.cpp

                  (!Current.Previous ||
                   Current.Previous->isNot(tok::l_square))) {
         Current.Type = TT_BinaryOperator;
       } else if (Current.is(tok::comment)) {
         if (Current.TokenText.startswith("//"))
           Current.Type = TT_LineComment;
         else
           Current.Type = TT_BlockComment;
       } else if (Current.is(tok::r_paren)) {
+        // FIXME: Pull cast detection into its own function.
         FormatToken *LeftOfParens = NULL;
         if (Current.MatchingParen)
           LeftOfParens = Current.MatchingParen->getPreviousNonComment();
         bool IsCast = false;
         bool ParensAreEmpty = Current.Previous == Current.MatchingParen;
         bool ParensAreType = !Current.Previous ||
                              Current.Previous->Type == TT_PointerOrReference ||
                              Current.Previous->Type == TT_TemplateCloser ||
                              Current.Previous->isSimpleTypeSpecifier();
         bool ParensCouldEndDecl =
             Current.Next &&
             Current.Next->isOneOf(tok::equal, tok::semi, tok::l_brace);
         bool IsSizeOfOrAlignOf =
             LeftOfParens &&
             LeftOfParens->isOneOf(tok::kw_sizeof, tok::kw_alignof);
         if (ParensAreType && !ParensCouldEndDecl && !IsSizeOfOrAlignOf &&
             ((Contexts.size() > 1 &&
               Contexts[Contexts.size() - 2].IsExpression) ||
              (Current.Next && Current.Next->isBinaryOperator())))
           IsCast = true;
-        if (Current.Next && Current.Next->isNot(tok::string_literal) &&
+        else if (Current.Next && Current.Next->isNot(tok::string_literal) &&
                  (Current.Next->Tok.isLiteral() ||
                   Current.Next->isOneOf(tok::kw_sizeof, tok::kw_alignof)))
           IsCast = true;
         // If there is an identifier after the (), it is likely a cast, unless
         // there is also an identifier before the ().
-        if (LeftOfParens && (LeftOfParens->Tok.getIdentifierInfo() == NULL ||
-                             LeftOfParens->is(tok::kw_return)) &&
-            LeftOfParens->Type != TT_OverloadedOperator &&
-            LeftOfParens->isNot(tok::at) &&
-            LeftOfParens->Type != TT_TemplateCloser && Current.Next &&
-            Current.Next->is(tok::identifier))
-          IsCast = true;
+        else if (LeftOfParens &&
+                 (LeftOfParens->Tok.getIdentifierInfo() == NULL ||
+                  LeftOfParens->is(tok::kw_return)) &&
+                 LeftOfParens->Type != TT_OverloadedOperator &&
+                 LeftOfParens->isNot(tok::at) &&
+                 LeftOfParens->Type != TT_TemplateCloser && Current.Next) {
+          if (Current.Next->isOneOf(tok::identifier, tok::numeric_constant)) {
+            IsCast = true;
+          } else {
+            // Use heuristics to recognize c style casting.
+            FormatToken *Prev = Current.Previous;
+            if (Prev && Prev->isOneOf(tok::amp, tok::star))
+              Prev = Prev->Previous;
+
+            if (Prev && Current.Next && Current.Next->Next) {
+              bool NextIsUnary = Current.Next->isUnaryOperator() ||
+                                 Current.Next->isOneOf(tok::amp, tok::star);
+              IsCast = NextIsUnary &&
+                       Current.Next->Next->isOneOf(tok::identifier,
+                                                   tok::numeric_constant);
+            }
+
+            for (; Prev != Current.MatchingParen; Prev = Prev->Previous) {
+              if (!Prev || !Prev->isOneOf(tok::kw_const, tok::identifier)) {
+                IsCast = false;
+                break;
+              }
+            }
+          }
+        }
         if (IsCast && !ParensAreEmpty)
           Current.Type = TT_CastRParen;
       } else if (Current.is(tok::at) && Current.Next) {
         switch (Current.Next->Tok.getObjCKeywordID()) {
         case tok::objc_interface:
         case tok::objc_implementation:
         case tok::objc_protocol:
           Current.Type = TT_ObjCDecl;

unittests/Format/FormatTest.cpp

   verifyFormat("int a = (int)sizeof(int);");
   verifyFormat("int a = (int)+2;");
   verifyFormat("my_int a = (my_int)2.0f;");
   verifyFormat("my_int a = (my_int)sizeof(int);");
   verifyFormat("return (my_int)aaa;");
   verifyFormat("#define x ((int)-1)");
   verifyFormat("#define p(q) ((int *)&q)");
 
-  // FIXME: Without type knowledge, this can still fall apart miserably.
-  verifyFormat("void f() { my_int a = (my_int) * b; }");
-  verifyFormat("void f() { return P ? (my_int) * P : (my_int)0; }");
-  verifyFormat("my_int a = (my_int) ~0;");
-  verifyFormat("my_int a = (my_int)++ a;");
-  verifyFormat("my_int a = (my_int) + 2;");
+  verifyFormat("void f() { my_int a = (my_int)*b; }");
+  verifyFormat("void f() { return P ? (my_int)*P : (my_int)0; }");
+  verifyFormat("my_int a = (my_int)~0;");
+  verifyFormat("my_int a = (my_int)++a;");
+  verifyFormat("my_int a = (my_int)+2;");
+  verifyFormat("my_int a = (my_int)1;");
+  verifyFormat("my_int a = (my_int *)1;");
+  verifyFormat("my_int a = (const my_int)-1;");
+  verifyFormat("my_int a = (const my_int *)-1;");
+
+  // FIXME: single value wrapped with paren will be treated as cast.
+  verifyFormat("void f(int i = (kValue)*kMask) {}");
 
   // Don't break after a cast's
   verifyFormat("int aaaaaaaaaaaaaaaaaaaaaaaaaaa =\n"
                "    (aaaaaaaaaaaaaaaaaaaaaaaaaa *)(aaaaaaaaaaaaaaaaaaaaaa +\n"
                "                                   bbbbbbbbbbbbbbbbbbbbbb);");
 
   // These are not casts.
   verifyFormat("void f(int *) {}");
   verifyFormat("f(foo)->b;");
   verifyFormat("f(foo).b;");
   verifyFormat("f(foo)(b);");
   verifyFormat("f(foo)[b];");
   verifyFormat("[](foo) { return 4; }(bar);");
   verifyFormat("(*funptr)(foo)[4];");
   verifyFormat("funptrs[4](foo)[4];");
   verifyFormat("void f(int *);");
   verifyFormat("void f(int *) = 0;");
   verifyFormat("void f(SmallVector<int>) {}");
   verifyFormat("void f(SmallVector<int>);");
   verifyFormat("void f(SmallVector<int>) = 0;");
-  verifyFormat("void f(int i = (kValue) * kMask) {}");
   verifyFormat("void f(int i = (kA * kB) & kMask) {}");
   verifyFormat("int a = sizeof(int) * b;");
   verifyFormat("int a = alignof(int) * b;", getGoogleStyle());
   verifyFormat("template <> void f<int>(int i) SOME_ANNOTATION;");
   verifyFormat("f(\"%\" SOME_MACRO(ll) \"d\");");
   verifyFormat("aaaaa &operator=(const aaaaa &) LLVM_DELETED_FUNCTION;");
 
   // These are not casts, but at some point were confused with casts.