Re: [webkit-dev] Type cast by using toFoo()
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_cast. - 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=rev&revision=158628). Regards, Ravi Kasibhatla. On Tue, Oct 1, 2013 at 11:22 AM, Gyuyoung Kim wrote: > 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=revision&revision=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 wrote: > >> On Mon, Sep 30, 2013 at 10:52 AM, Yong Li wrote: >> >>> >>> 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
Re: [webkit-dev] Type cast by using toFoo()
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=revision&revision=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 wrote: > On Mon, Sep 30, 2013 at 10:52 AM, Yong Li wrote: > >> >> 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
Re: [webkit-dev] Type cast by using toFoo()
On Mon, Sep 30, 2013 at 10:52 AM, Yong Li wrote: > > 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()
Bottom line is turning on RTTI in debug build? From: Darin Adler<mailto:da...@apple.com> Sent: 2013-09-30 1:50 PM To: Yong Li<mailto:yong.li.web...@outlook.com> Cc: Gyuyoung Kim<mailto:gyuyoung@webkit.org>; Sam Weinig<mailto:wei...@apple.com>; WebKit Development<mailto:webkit-dev@lists.webkit.org> Subject: Re: [webkit-dev] Type cast by using toFoo() On Sep 30, 2013, at 9:54 AM, Yong Li 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()
On Sep 30, 2013, at 9:54 AM, Yong Li 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()
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 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()
On Sep 30, 2013, at 3:39 AM, Gyuyoung Kim 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] Type cast by using toFoo()
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
