Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23654034:
Add code style check error for using static_cast instead of toFoo function. (Closed)
|
Created:
7 years, 3 months ago by r.kasibhatla Modified:
7 years, 3 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, ojan, aedla, ianbeer, Martin Barbella, abarth-chromium Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionAdd code style check error for using static_cast instead of toFoo function.
We are migrating to new syntax of calling toFoo for any type of static_cast we do on Foo objects. We are adding to more and more classes toFoo helper functions, so we should ensure that the added helpers are actually *used* in the code. Currently, there is no enforcement on the same and through this change we are adding that style check which will fail any patch upload if we are not following toFoo style (if toFoo exists). As part of testing, executed check-webkit-style on all source files which still have static_cast<Foo*> code syntax. The script throws style error if we are using static_cast though we already have toFoo helper function. If toFoo is not yet defined, it doesn't throw any error right now (should that be changed?)
BUG=None
TEST=none; no behavior change;
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=158059
Patch Set 1 : #
Total comments: 10
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 1
Messages
Total messages: 25 (0 generated)
PTAL.
PTAL
Inferno will be happy. :)
I'm OK with the idea. Ojan or dpranke are more our guardians of python these days.
Wow! you are security team's best friend from now on :) We had this item on our todo list for this quarter. We will also add this check for chromium code later.
https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3477: matched = search(r'\bstatic_cast<(\w+\s?\*+\s?)>', line) I think you should cover other variants like static_pointer_cast as well. You are not checking if the static_cast is part of a toFoo* function. In that case, it will show a wrong error. https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3487: if is_func_defined: We are showing the error only if toFoo* function is defined. Atleast 50% of the times, people don't use toFoo function because it is not defined. So, we want to encourage them to define it if it does not exist already. https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3489: 'Consider using to%s helper function in %s instead of static_cast<%s>' % change "Consider using" to a more stronger message. We don't want to allow this pattern at all.
On 2013/09/16 16:56:46, inferno wrote: > https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... > File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): > > https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... > Tools/Scripts/webkitpy/style/checkers/cpp.py:3477: matched = > search(r'\bstatic_cast<(\w+\s?\*+\s?)>', line) > I think you should cover other variants like static_pointer_cast as well. Sure. I wasn't aware that static_pointer_cast was actively being used in WebKit code as well. Will add for those patterns as well. > You are not checking if the static_cast is part of a toFoo* function. In that > case, it will show a wrong error. Aah. Thanks for catching the error. :). Will filter static_cast used in toFoo functions itself. > > https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... > Tools/Scripts/webkitpy/style/checkers/cpp.py:3487: if is_func_defined: > We are showing the error only if toFoo* function is defined. Atleast 50% of the > times, people don't use toFoo function because it is not defined. So, we want to > encourage them to define it if it does not exist already. Sure, will update in the new patch. > > https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... > Tools/Scripts/webkitpy/style/checkers/cpp.py:3489: 'Consider using to%s helper > function in %s instead of static_cast<%s>' % > change "Consider using" to a more stronger message. We don't want to allow this > pattern at all. Sure, will definately update the message to *force* define toFoo (if reqd) else use the same. :)
Basically looks okay, python-wise. I noted a couple of nits, and inferno noted some other things. Once you've made him happy, lgtm :). https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3449: # List of all geninuine alternatives to static_cast. Nit: typo, "genuine". https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3456: #r = re.compile(r'%s(\s+)%s(\(\w+)%s(\*+\))' % ("void", pattern, object_class)) Delete this commented-out line?
https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3449: # List of all geninuine alternatives to static_cast. On 2013/09/17 01:54:55, Dirk Pranke wrote: > Nit: typo, "genuine". Done. https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3456: #r = re.compile(r'%s(\s+)%s(\(\w+)%s(\*+\))' % ("void", pattern, object_class)) On 2013/09/17 01:54:55, Dirk Pranke wrote: > Delete this commented-out line? Done. https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3477: matched = search(r'\bstatic_cast<(\w+\s?\*+\s?)>', line) On 2013/09/16 16:56:46, inferno wrote: > I think you should cover other variants like static_pointer_cast as well. I was looking at including static_pointer_cast in the style check, but I am not sure how do we enforce it. static_pointer_cast is defined only as part of templates RefPtr, PassRefPtr, OwnPtr & PassOwnPtr and it returns a RefPtr, PassRefPtr, OwnPtr or PassOwnPtr respectively. Unlike static_cast which returns separate value for each toFoo instance, static_pointer_cast always returns one of the 4 types which the template parameter changing. So, I am not sure if we raise the flag on static_pointer_cast, how would the user define its corresponding toFoo function. Any suggestions? > You are not checking if the static_cast is part of a toFoo* function. In that > case, it will show a wrong error. Done. https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3487: if is_func_defined: On 2013/09/16 16:56:46, inferno wrote: > We are showing the error only if toFoo* function is defined. Atleast 50% of the > times, people don't use toFoo function because it is not defined. So, we want to > encourage them to define it if it does not exist already. Done. https://codereview.chromium.org/23654034/diff/5001/Tools/Scripts/webkitpy/sty... Tools/Scripts/webkitpy/style/checkers/cpp.py:3489: 'Consider using to%s helper function in %s instead of static_cast<%s>' % On 2013/09/16 16:56:46, inferno wrote: > change "Consider using" to a more stronger message. We don't want to allow this > pattern at all. Done.
lgtm https://codereview.chromium.org/23654034/diff/17001/Tools/Scripts/webkitpy/st... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/23654034/diff/17001/Tools/Scripts/webkitpy/st... Tools/Scripts/webkitpy/style/checkers/cpp.py:3287: # Check for usage of static_cast<Classname*> or static_pointer_cast(). remove "or static_pointer_cast()". I have an idea for covering static_pointer_cast (please do this in a seperate bug). Since it is a template wrapper, why not we just add ASSERT_WITH_SECURITY_IMPLICATION in the wrapper function itself. Of course, that would mean fix some compile failures, since isT() won't exist in some classes, but once it is done, it will automatically work and makes a compile failure if isT() does not exist (for a new patch). https://codereview.chromium.org/23654034/diff/17001/Tools/Scripts/webkitpy/st... Tools/Scripts/webkitpy/style/checkers/cpp.py:3491: class_name = re.sub('[\*]', '', matched.group(1)) You need to parse ::. otherwise things like 'static_cast<WebCore::HTMLDocument*>' will fail.
Corrected for checking namespace::classname. PTAL. https://codereview.chromium.org/23654034/diff/17001/Tools/Scripts/webkitpy/st... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/23654034/diff/17001/Tools/Scripts/webkitpy/st... Tools/Scripts/webkitpy/style/checkers/cpp.py:3287: # Check for usage of static_cast<Classname*> or static_pointer_cast(). On 2013/09/18 16:46:04, inferno wrote: > remove "or static_pointer_cast()". I have an idea for covering > static_pointer_cast (please do this in a seperate bug). Since it is a template > wrapper, why not we just add ASSERT_WITH_SECURITY_IMPLICATION in the wrapper > function itself. Of course, that would mean fix some compile failures, since > isT() won't exist in some classes, but once it is done, it will automatically > work and makes a compile failure if isT() does not exist (for a new patch). Done. https://codereview.chromium.org/23654034/diff/17001/Tools/Scripts/webkitpy/st... Tools/Scripts/webkitpy/style/checkers/cpp.py:3491: class_name = re.sub('[\*]', '', matched.group(1)) On 2013/09/18 16:46:04, inferno wrote: > You need to parse ::. otherwise things like > 'static_cast<WebCore::HTMLDocument*>' will fail. Done.
https://codereview.chromium.org/23654034/diff/24001/Tools/Scripts/webkitpy/st... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/23654034/diff/24001/Tools/Scripts/webkitpy/st... Tools/Scripts/webkitpy/style/checkers/cpp.py:3497: namespace_pos = class_name.find(':') Do a rfind and then class_name = class_name[namespace_pos + 1:] Otherwise you will have wrong results for A::B::C
Corrected the find.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/23654034/30001
Message was sent while issue was closed.
Change committed as 158059
Message was sent while issue was closed.
This warning is not playing nice with the supplement-pattern code, which currently uses static_cast: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Presumably we could make a helper for interfacing with Supplements which would avoid code from needing a static_cast here. But toSupplementType(Suplement<Type>*) isn't the answer here.
Message was sent while issue was closed.
I think we should consider turnign this check off for now. And running it over the entire codebase so we can understand where the current errors are (there will be lots and we don't need to fix them all), but that will help inform us of how much pain we're going to cause folks as they run into this trying to modify existing code.
Message was sent while issue was closed.
On 2013/09/20 18:38:06, eseidel wrote: > I think we should consider turnign this check off for now. And running it over > the entire codebase so we can understand where the current errors are (there > will be lots and we don't need to fix them all), but that will help inform us of > how much pain we're going to cause folks as they run into this trying to modify > existing code. So I fixed one issue in https://codereview.chromium.org/24227005/ but as eseidel writes, we should have tests for this.
Message was sent while issue was closed.
On 2013/09/20 19:00:18, alecflett wrote: > On 2013/09/20 18:38:06, eseidel wrote: > > I think we should consider turnign this check off for now. And running it > over > > the entire codebase so we can understand where the current errors are (there > > will be lots and we don't need to fix them all), but that will help inform us > of > > how much pain we're going to cause folks as they run into this trying to > modify > > existing code. > > So I fixed one issue in https://codereview.chromium.org/24227005/ but as eseidel > writes, we should have tests for this. Can we turn this into a warning for now, instead of a blocker ? That way, we will still prevent badness. I think Supplement might be the only case (since i encountered this while writing my bad cast catcher script in https://code.google.com/p/chromium/issues/detail?id=277650).
Message was sent while issue was closed.
I'm sorry, I didn't notice this before but this implementation and unit test has a few problems. I'm gonna revert this for now since it breaks test-webkitpy for me locally. Also, I think this broke on the WebKit XP bot, but I'm not sure why ... http://build.chromium.org/p/chromium.webkit/builders/WebKit%20XP/builds/7113 https://chromiumcodereview.appspot.com/23654034/diff/30001/Tools/Scripts/webk... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://chromiumcodereview.appspot.com/23654034/diff/30001/Tools/Scripts/webk... Tools/Scripts/webkitpy/style/checkers/cpp.py:3440: start_dir = os.path.dirname(os.path.abspath(filename)) Unfortunately, I think this breaks if you only have an unqualified header and you are running the unit tests from a directory other than third_party/WebKit. Please fix this. Also, using os.walk to crawl looking for the header in question is not so good. At the very least, you can't mock this out for test purposes. More seriously, this is kind of an expensive thing to do. There are other places in the unittests where we stub out os-level functions to get around this, you can look for examples there.
Message was sent while issue was closed.
http://crbug.com/295673 is also related to this change.
Message was sent while issue was closed.
On 2013/09/20 22:34:41, Dirk Pranke wrote: > I'm sorry, I didn't notice this before but this implementation and unit test has > a few problems. I'm gonna revert this for now since it breaks test-webkitpy for > me locally. > > Also, I think this broke on the WebKit XP bot, but I'm not sure why ... > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%2520XP/builds/7113 > > https://chromiumcodereview.appspot.com/23654034/diff/30001/Tools/Scripts/webk... > File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): > > https://chromiumcodereview.appspot.com/23654034/diff/30001/Tools/Scripts/webk... > Tools/Scripts/webkitpy/style/checkers/cpp.py:3440: start_dir = > os.path.dirname(os.path.abspath(filename)) > Unfortunately, I think this breaks if you only have an unqualified header and > you are running the unit tests from a directory other than third_party/WebKit. > Please fix this. > > Also, using os.walk to crawl looking for the header in question is not so good. > At the very least, you can't mock this out for test purposes. More seriously, > this is kind of an expensive thing to do. > > There are other places in the unittests where we stub out os-level functions to > get around this, you can look for examples there. Sure, thanks for pointing it out. I will check for the examples in the codebase.
Message was sent while issue was closed.
On 2013/09/20 19:06:15, aarya wrote: > On 2013/09/20 19:00:18, alecflett wrote: > > On 2013/09/20 18:38:06, eseidel wrote: > > > I think we should consider turnign this check off for now. And running it > > over > > > the entire codebase so we can understand where the current errors are (there > > > will be lots and we don't need to fix them all), but that will help inform > us > > of > > > how much pain we're going to cause folks as they run into this trying to > > modify > > > existing code. > > > > So I fixed one issue in https://codereview.chromium.org/24227005/ but as > eseidel > > writes, we should have tests for this. > > Can we turn this into a warning for now, instead of a blocker ? That way, we > will still prevent badness. I think Supplement might be the only case (since i > encountered this while writing my bad cast catcher script in > https://code.google.com/p/chromium/issues/detail?id=277650). I am ok with that as long as reviewers are fine with it. Just one question - how do make it as warning? Should it put in any specific filter for doing that? |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
