Bug 122156 – [Tools] Add the check to style-checker for reporting use of static_cast on typename objects.

• Status: RESOLVED
• Resolution: INVALID
• Component: Tools / Tests

[Ravi Kasibhatla]

As part of new changes (added by Kling/Gyuyoung et. all), WebKit is migrating to new syntax of calling toFoo() instead of doing static_cast on Foo objects and more and more classes are being changed to add toFoo helper functions. There should be a way to ensure that the added helpers are actually *used* in the code. Currently, there is no enforcement to use the added toFoo functions nor we exert pressure on the dev to add new toFoo() if required in some new module. We should add this check to style checker and try to *enforce* rigidly the use/addition of toFoo().
For now we can keep the change under *disabled* filter and once all the codebase is changed to use toFoo() we should report a style error if the uploaded patch contains static_cast on any object.

[Ravi Kasibhatla]

I have committed the patch which addresses this change in blink under the CL (https://src.chromium.org/viewvc/blink?view=rev&revision=158628).

My logic for checking the error is simple:
- In the patch, I check for any line which contains regex static_cast<Foo*>.
- If line does contain it, I pick the regex part Foo and search for Foo.h in the codebase.
- In Foo.h, I check for toFoo signature and if it is present, I ask the user to use it. If the signature is not present, I throw the message of adding the toFoo() and using it.
- In cases, where Foo.h is not found, I don't report any error (which I plan to look in future for more refining).

I do need to modify the patch for WebKit to accommodate the use of macros (while checking for presence of toFoo() in the header file) like ELEMENT_TYPE_CASTS and CSS_VALUE_TYPE_CASTS which are used to generate toFoo at compilation times.

[Ravi Kasibhatla]

[Merging from Blink]

[Ravi Kasibhatla]

Created attachment 213079 [details]
Patch

[Darin Adler]

Comment on attachment 213079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=213079&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:780
> +        self.assertEqual(message, 'static_cast of class objects is not allowed. Use toFoo defined in Foo.h.'

This message is not clear. What are "class objects"?

> Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:789
> +        self.assertEqual(message, 'static_cast of class objects is not allowed. Add toFoo in Foo.h and use it instead.'

Ditto.

[Ravi Kasibhatla]

Created attachment 213230 [details]
Patch

[Ravi Kasibhatla]

Created attachment 213232 [details]
Patch

[Ravi Kasibhatla]

(In reply to comment #4)
> (From update of attachment 213079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213079&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:780
> > +        self.assertEqual(message, 'static_cast of class objects is not allowed. Use toFoo defined in Foo.h.'
> 
> This message is not clear. What are "class objects"?
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:789
> > +        self.assertEqual(message, 'static_cast of class objects is not allowed. Add toFoo in Foo.h and use it instead.'
> 
> Ditto.
Done.

[Ravi Kasibhatla]

(In reply to comment #7)
> (In reply to comment #4)
> > (From update of attachment 213079 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=213079&action=review
> > 
> > > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:780
> > > +        self.assertEqual(message, 'static_cast of class objects is not allowed. Use toFoo defined in Foo.h.'
> > 
> > This message is not clear. What are "class objects"?
> > 
> > > Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:789
> > > +        self.assertEqual(message, 'static_cast of class objects is not allowed. Add toFoo in Foo.h and use it instead.'
> > 
> > Ditto.
> Done.
Thoughts on the patch?

[Darin Adler]

Comment on attachment 213232 [details]
Patch

The rule this patch enforces might be too strict. I can’t tell by reading the code. When you run this check across existing code, do you get any false positives, errors on lines of code that are not incorrect?

[Darin Adler]

Maybe you could upload the list of failures this finds when run on our current code so we can try to spot false positives?

[Daniel Bates]

Comment on attachment 213232 [details]
Patch

Thank you Ravi for the patch. As it turns out we now use downcast<X>() to downcast a type instead of static_cast<X>() or toX(). It would be nice if style checker could suggest using downcast<X>() instead of static_cast<X>(). This is likely non-trivial to implement.

[Daniel Bates]

I am marking this Resolved/Invalid because we prefer using downcast<X>() instead of static_cast<X>() to toX(). If there is interest in teaching the style checker to suggest use of downcast<X>() then I suggest we file a new bug.

[Daniel Bates]

(In reply to comment #12)
> I am marking this Resolved/Invalid because we prefer using downcast<X>()
> instead of static_cast<X>() to toX()

*instead of static_cast<X>() or toX()