Re: [webkit-dev] Type cast by using toFoo()

2014-01-20 Thread Ravi Kasibhatla
I am the original author of style checker patch reported in previous mail
by Gyuyoung.

My logic for checking the error is simple:
- In the patch, I check for any line which contains regex static_castFoo*.
- 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 have raised the bug https://bugs.webkit.org/show_bug.cgi?id=122156 for
tracking this change in WebKit and will soon be porting the patch which got
committed in blink (
https://src.chromium.org/viewvc/blink?view=revrevision=158628).

Regards,
Ravi Kasibhatla.


On Tue, Oct 1, 2013 at 11:22 AM, Gyuyoung Kim gyuyoung@webkit.orgwrote:

 My plan is to show style error when submitted patch doesn't use toFoo()
 though toFoo exists. This idea was originated from blink commit. However,
 it was reverted because of some regression.

 http://src.chromium.org/viewvc/blink?view=revisionrevision=158059


 If my understanding is correct, the toFoo() style checker checks if there
 is toFoo() in a class. If uploaded patch uses static_cast instead of
 toFoo() though there is toFoo(), style checker will generate style error.

 Anyway, I think I need to investigate the commit and consider the idea
 further.

 Gyuyoung.


 On Tue, Oct 1, 2013 at 3:20 AM, Ryosuke Niwa rn...@webkit.org wrote:

 On Mon, Sep 30, 2013 at 10:52 AM, Yong Li yong.li.web...@outlook.comwrote:


 Bottom line is turning on RTTI in debug build?


 Style checker analyzes the code statically.  It's nothing to do with
 runtime assertions.  If that wasn't clear enough, style check happens
 before WebKit is ever built.

 - R. Niwa


 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org
 https://lists.webkit.org/mailman/listinfo/webkit-dev



 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org
 https://lists.webkit.org/mailman/listinfo/webkit-dev


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


[webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Gyuyoung Kim
Hello WebKittens,

I have focused on using toFoo() for SVG and CSS instead of using
static_cast.  Because I think there are some advantages when we use it.

  - Bad type cast can be detected by using ASSERTION in toFoo(). The
toFoo() function has an ASSERTION to check if source value is a proper
super class.
  - Unnecessary local variables can be removed. There are some local
variables, which are only used once in WebKit. In those cases, we don’t
need to use a local variable. Besides, we can remove unnecessary ASSERTION
because toFoo() already has it.
  - I believe toFoo() can improve code readability.

Currently, HTML, SVG Elements support toHTML|SVGFooElement() and CSSValue
also starts to support toCSSFooValue(). Please check if there is toFoo()
when you need to use static_cast in HTML, SVG and CSS module.

Finally I plan to add this toFoo() policy to the WebKit style checker.

Gyuyoung.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Sam Weinig

On Sep 30, 2013, at 3:39 AM, Gyuyoung Kim gyuyoung@webkit.org wrote:

 Hello WebKittens,
 
 I have focused on using toFoo() for SVG and CSS instead of using 
 static_cast.  Because I think there are some advantages when we use it.
 
   - Bad type cast can be detected by using ASSERTION in toFoo(). The toFoo() 
 function has an ASSERTION to check if source value is a proper super class.
   - Unnecessary local variables can be removed. There are some local 
 variables, which are only used once in WebKit. In those cases, we don’t need 
 to use a local variable. Besides, we can remove unnecessary ASSERTION because 
 toFoo() already has it.
   - I believe toFoo() can improve code readability.
 
 Currently, HTML, SVG Elements support toHTML|SVGFooElement() and CSSValue 
 also starts to support toCSSFooValue(). Please check if there is toFoo() when 
 you need to use static_cast in HTML, SVG and CSS module.

Nice.

 
 Finally I plan to add this toFoo() policy to the WebKit style checker.

Can you explain more about this?  How are you going to determine static_casts 
that are acceptable from ones that aren’t.

-Sam

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Yong Li
I think it can be done by checking the vtable pointer if the classes are 
virtual.



From: Sam Weinig
Sent: ‎Monday‎, ‎September‎ ‎30‎, ‎2013 ‎12‎:‎53‎ ‎PM
To: Gyuyoung Kim
Cc: Webkit Development List



On Sep 30, 2013, at 3:39 AM, Gyuyoung Kim gyuyoung@webkit.org wrote:

 Hello WebKittens,
 
 I have focused on using toFoo() for SVG and CSS instead of using 
 static_cast.  Because I think there are some advantages when we use it.
 
   - Bad type cast can be detected by using ASSERTION in toFoo(). The toFoo() 
 function has an ASSERTION to check if source value is a proper super class.
   - Unnecessary local variables can be removed. There are some local 
 variables, which are only used once in WebKit. In those cases, we don’t need 
 to use a local variable. Besides, we can remove unnecessary ASSERTION because 
 toFoo() already has it.
   - I believe toFoo() can improve code readability.
 
 Currently, HTML, SVG Elements support toHTML|SVGFooElement() and CSSValue 
 also starts to support toCSSFooValue(). Please check if there is toFoo() when 
 you need to use static_cast in HTML, SVG and CSS module.

Nice.

 
 Finally I plan to add this toFoo() policy to the WebKit style checker.

Can you explain more about this?  How are you going to determine static_casts 
that are acceptable from ones that aren’t.

-Sam

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Darin Adler
On Sep 30, 2013, at 9:54 AM, Yong Li yong.li.web...@outlook.com wrote:

  Finally I plan to add this toFoo() policy to the WebKit style checker.
 
 Can you explain more about this?  How are you going to determine 
 static_casts that are acceptable from ones that aren’t.
 
 I think it can be done by checking the vtable pointer if the classes are 
 virtual.

The style checker runs on source code, so it can’t check things like vtable 
pointers.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Yong Li

Bottom line is turning on RTTI in debug build?

From: Darin Adlermailto:da...@apple.com
Sent: ‎2013-‎09-‎30 1:50 PM
To: Yong Limailto:yong.li.web...@outlook.com
Cc: Gyuyoung Kimmailto:gyuyoung@webkit.org; Sam 
Weinigmailto:wei...@apple.com; WebKit 
Developmentmailto:webkit-dev@lists.webkit.org
Subject: Re: [webkit-dev] Type cast by using toFoo()

On Sep 30, 2013, at 9:54 AM, Yong Li yong.li.web...@outlook.com wrote:

  Finally I plan to add this toFoo() policy to the WebKit style checker.

 Can you explain more about this?  How are you going to determine 
 static_casts that are acceptable from ones that aren’t.

 I think it can be done by checking the vtable pointer if the classes are 
 virtual.

The style checker runs on source code, so it can’t check things like vtable 
pointers.

— Darin
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Ryosuke Niwa
On Mon, Sep 30, 2013 at 10:52 AM, Yong Li yong.li.web...@outlook.comwrote:


 Bottom line is turning on RTTI in debug build?


Style checker analyzes the code statically.  It's nothing to do with
runtime assertions.  If that wasn't clear enough, style check happens
before WebKit is ever built.

- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev


Re: [webkit-dev] Type cast by using toFoo()

2013-09-30 Thread Gyuyoung Kim
My plan is to show style error when submitted patch doesn't use toFoo()
though toFoo exists. This idea was originated from blink commit. However,
it was reverted because of some regression.

http://src.chromium.org/viewvc/blink?view=revisionrevision=158059


If my understanding is correct, the toFoo() style checker checks if there
is toFoo() in a class. If uploaded patch uses static_cast instead of
toFoo() though there is toFoo(), style checker will generate style error.

Anyway, I think I need to investigate the commit and consider the idea
further.

Gyuyoung.


On Tue, Oct 1, 2013 at 3:20 AM, Ryosuke Niwa rn...@webkit.org wrote:

 On Mon, Sep 30, 2013 at 10:52 AM, Yong Li yong.li.web...@outlook.comwrote:


 Bottom line is turning on RTTI in debug build?


 Style checker analyzes the code statically.  It's nothing to do with
 runtime assertions.  If that wasn't clear enough, style check happens
 before WebKit is ever built.

 - R. Niwa


 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org
 https://lists.webkit.org/mailman/listinfo/webkit-dev


___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev