Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
145033013:
Update the static_cast style check to check for DEFINE_TYPE_CASTS patterns as well. (Closed)
|
Created:
6 years, 11 months ago by r.kasibhatla Modified:
6 years, 10 months ago CC:
blink-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionUpdate the static_cast style check to check for DEFINE_TYPE_CASTS patterns as well.
Existing implementation of check-webkit-style warns the user if he uses static_cast<>
in the code. This is behind runtime/casting filter which needs to be enabled specifically.
As part of 309516, all the toFoo() definitions have been replaced with DEFINE_*_TYPE_CASTS
macro, which defines the toFoo functions at compile time. This patch updates the check to
include even the macro pattern to identify whether existing toFoo exists or user needs to
define the toFoo convienience function (through macro).
Since all existing usage of static_cast<Foo*> has been replaced with toFoo() calls,
now we are throwing an error if user uses static_cast<> inspite of presence of toFoo().
If toFoo doesn't exist, we still retain a warning behind runtime/casting filter.
BUG=309516
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166491
Patch Set 1 #
Total comments: 19
Patch Set 2 : Corrected the nits! #Patch Set 3 : Corrected the message! #
Total comments: 1
Messages
Total messages: 24 (0 generated)
PTAL.
On 2014/01/24 15:46:43, r.kasibhatla wrote: > PTAL. PTAL.
mostly rubber-stamp LGTM. I wish we had someone who was more of an OWNER for this code better these days. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3419: search_func_name: The toFoo convinience function string. Nit: "convenience". https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3441: # DEFINE_TYPE_CASTS pattern not found. Search for toFoo pattern instead in the file. Nit: it's a bit odd to me to have a comment immediately preceding an elif at the same indentation level as is if and elif (it makes it a bit harder to understand which lines are executed when), so I'd probably move this comment after line 3442 and indent it (and possibly reword it a bit). https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3534: # FIXME(kphanee): Handle cases where Classname might be defined in some other header or cpp file. Nit: We don't use usernames in FIXMEs. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:762: # TODO (kphanee): Add cases for testing the DEFINE_TYPE_CASTS usecases also. NIt: s/TODO/FIXME, don't use a username.
lgtm https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3414: def check_for_toFoo_definition(filename, search_func_name, grep_patterns, error): check_for_toFoo_definition -> check_for_cast_definition since now we support DEFINE_TYPE_CASTS search_func_name -> search_function_name (good to not abbreviate). https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3419: search_func_name: The toFoo convinience function string. typo: convenience https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3439: if re.search(grep_patterns[0], line): grep_patterns[0], grep_patterns[1] is not a good way to use here. If someone modifies the order, this becomes a problem. Why not just define them here. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3535: # It would remove this hack for ignoring not finding Foo.h This line is not clear, can you rephrase this. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:777: self.assertEqual(message, 'static_cast of class objects is not allowed. Use toFoo defined in Foo.h.' nit: do we want to add DEFINE_TYPE_CASTS in message here.
I will try to put another version of patch which will correct the below FIXME also. This fixme looks ugly for me. :p # FIXME: Handle cases where Classname might be defined in some other header or cpp file. # It will ensure check_for_toFoo_definition nevers 'None' and we can remove this unneccessary check. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3414: def check_for_toFoo_definition(filename, search_func_name, grep_patterns, error): On 2014/01/27 19:45:56, aarya wrote: > check_for_toFoo_definition -> check_for_cast_definition since now we support > DEFINE_TYPE_CASTS > > search_func_name -> search_function_name (good to not abbreviate). Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3419: search_func_name: The toFoo convinience function string. On 2014/01/27 19:38:51, Dirk Pranke wrote: > Nit: "convenience". Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3419: search_func_name: The toFoo convinience function string. On 2014/01/27 19:45:56, aarya wrote: > typo: convenience Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3439: if re.search(grep_patterns[0], line): On 2014/01/27 19:45:56, aarya wrote: > grep_patterns[0], grep_patterns[1] is not a good way to use here. If someone > modifies the order, this becomes a problem. Why not just define them here. Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3441: # DEFINE_TYPE_CASTS pattern not found. Search for toFoo pattern instead in the file. On 2014/01/27 19:38:51, Dirk Pranke wrote: > Nit: it's a bit odd to me to have a comment immediately preceding an elif at the > same indentation level as is if and elif (it makes it a bit harder to understand > which lines are executed when), so I'd probably move this comment after line > 3442 and indent it (and possibly reword it a bit). Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3534: # FIXME(kphanee): Handle cases where Classname might be defined in some other header or cpp file. On 2014/01/27 19:38:51, Dirk Pranke wrote: > Nit: We don't use usernames in FIXMEs. Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp.py:3535: # It would remove this hack for ignoring not finding Foo.h On 2014/01/27 19:45:56, aarya wrote: > This line is not clear, can you rephrase this. Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:762: # TODO (kphanee): Add cases for testing the DEFINE_TYPE_CASTS usecases also. On 2014/01/27 19:38:51, Dirk Pranke wrote: > NIt: s/TODO/FIXME, don't use a username. Done. https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:777: self.assertEqual(message, 'static_cast of class objects is not allowed. Use toFoo defined in Foo.h.' On 2014/01/27 19:45:56, aarya wrote: > nit: do we want to add DEFINE_TYPE_CASTS in message here. This usecase is specifically for the error when we already have the toFoo defined but we are not using it. Though, for the 2nd usecase, when toFoo is not yet defined, I agree that we should encourage to define using DEFINE_TYPE_CASTS. But not sure what exact message would look nice. Would something like this sounds nice - "static_cast of class objects is not allowed. Define cast macro DEFINE_TYPE_CASTS(Foo) in Foo.h and use it."
lgtm https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... File Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py (right): https://codereview.chromium.org/145033013/diff/1/Tools/Scripts/webkitpy/style... Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py:777: self.assertEqual(message, 'static_cast of class objects is not allowed. Use toFoo defined in Foo.h.' On 2014/02/03 15:29:15, r.kasibhatla wrote: > On 2014/01/27 19:45:56, aarya wrote: > > nit: do we want to add DEFINE_TYPE_CASTS in message here. > This usecase is specifically for the error when we already have the toFoo > defined but we are not using it. > > Though, for the 2nd usecase, when toFoo is not yet defined, I agree that we > should encourage to define using DEFINE_TYPE_CASTS. But > not sure what exact message would look nice. > Would something like this sounds nice - > "static_cast of class objects is not allowed. Define cast macro > DEFINE_TYPE_CASTS(Foo) in Foo.h and use it." Yes, this new message is fine.
The CQ bit was checked by r.kasibhatla@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/145033013/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
The CQ bit was checked by r.kasibhatla@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/145033013/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
The CQ bit was checked by r.kasibhatla@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/145033013/110001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
The CQ bit was checked by r.kasibhatla@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/145033013/110001
Message was sent while issue was closed.
Change committed as 166491
Message was sent while issue was closed.
https://codereview.chromium.org/145033013/diff/110001/Tools/Scripts/webkitpy/... File Tools/Scripts/webkitpy/style/checkers/cpp.py (right): https://codereview.chromium.org/145033013/diff/110001/Tools/Scripts/webkitpy/... Tools/Scripts/webkitpy/style/checkers/cpp.py:3533: _RE_GENERAL_MACRO_PATTERN_STRING = re.compile(r'\s*DEFINE_(\w*)TYPE_CASTS\(%s,(.+)(\);$)' % class_name) Doesn't this need a 'global' to be able to work as intended?
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/153953009/ by r.kasibhatla@samsung.com. The reason for reverting is: The patch will be breaking the style check on the patch since the regex compiled _RE_GENERAL_MACRO_PATTERN_STRING & _RE_GENERAL_MACRO_PATTERN_STRING are not correct. The regex (mentioned above) needs to be set properly. Will be uploading new patch with correct regex.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
