Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
653883002:
Adding Presubmit error when OVERRIDE and FINAL is not used as C++11 standard (Closed)
DescriptionAdding Presubmit error when OVERRIDE and FINAL is not used as C++11 standard
Alerting the error for use to use c++11 constructs |override| and |final|
instead of OVERRIDE and FINAL respctively
BUG=417463
Committed: https://crrev.com/f21db965b70b61dea512fc6e98aad6e919274610
Cr-Commit-Position: refs/heads/master@{#299880}
Patch Set 1 #Patch Set 2 : Finetuned the patch #
Total comments: 2
Patch Set 3 : Finetuned patch as per suggestions #
Total comments: 2
Patch Set 4 : Addressed the comments #
Total comments: 12
Patch Set 5 : Incorporated changes and add unittests #
Total comments: 9
Patch Set 6 : Applied NIT Changes #
Total comments: 2
Patch Set 7 : Applied sys changes and sort order #
Total comments: 2
Patch Set 8 : incorporated review comments #
Total comments: 2
Patch Set 9 : incorporated review comments #
Total comments: 2
Patch Set 10 : Final changes #Messages
Total messages: 29 (4 generated)
mohan.reddy@samsung.com changed reviewers: + avi@chromium.org, mkwst@chromium.org
In this patch I have covered the cases to catch presubmit errors. Cases like lowercase and upper case and strings with macros different combination with OVERRIDE like OVERIDE_CHROMIUM, FINAL_CHROMIUM etc. PTAL
On 2014/10/14 09:44:57, MRV wrote: > In this patch I have covered the cases to catch presubmit errors. > Cases like lowercase and upper case and strings with macros different > combination with OVERRIDE like OVERIDE_CHROMIUM, FINAL_CHROMIUM etc. > > PTAL This patch looks fine, but please add a test to 'PRESUBMIT_test.py' with the cases you've considered.
https://codereview.chromium.org/653883002/diff/20001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/20001/PRESUBMIT.py#newcode1270 PRESUBMIT.py:1270: if (re.search(r"\b" + override_pattern + r"\b", line) or I'd suggest simply inlining the patterns here: r"\bOVERRIDE\b" is perfectly legible.
Dear Mr. Mike, thanks for your feedback, its working well. I have updated the changes in patch-set3 PTAL. https://codereview.chromium.org/653883002/diff/20001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/20001/PRESUBMIT.py#newcode1270 PRESUBMIT.py:1270: if (re.search(r"\b" + override_pattern + r"\b", line) or On 2014/10/14 10:35:54, Mike West wrote: > I'd suggest simply inlining the patterns here: r"\bOVERRIDE\b" is perfectly > legible. Done.
Could you upload your changes to 'PRESUBMIT_test.py', please? https://codereview.chromium.org/653883002/diff/40001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/40001/PRESUBMIT.py#newcode1265 PRESUBMIT.py:1265: final_pattern = "FINAL" You don't need these local variables anymore, now that you've inlined the values.
Dear Mr. Mike thanks for your time and review. I have incorporated changes in the patch-set4. PTAL https://codereview.chromium.org/653883002/diff/40001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/40001/PRESUBMIT.py#newcode1265 PRESUBMIT.py:1265: final_pattern = "FINAL" On 2014/10/14 11:53:16, Mike West wrote: > You don't need these local variables anymore, now that you've inlined the > values. Done.
You still haven't uploaded the tests I asked for. :) I'd appreciate it if you could take a look at 'PRESUBMIT_test.py', and add the test cases for this check. They should end up looking similar to the IncludeOrderTest tests in that file.
maruel@chromium.org changed reviewers: + maruel@chromium.org
https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode12 PRESUBMIT.py:12: import re I know it's not your fault but it's really time to remove this. Please move to inside GetPreferredTryMasters() then fix any remaining caller to use input_api.re. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode13 PRESUBMIT.py:13: import sys Same. sys.executable is accessible through input_api.executable. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1261 PRESUBMIT.py:1261: two lines between file level symbols. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1268 PRESUBMIT.py:1268: if (re.search(r"\bOVERRIDE\b", line) or input_api.re.search(r"\b(FINAL|OVERRIDE)\b", line) https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1274 PRESUBMIT.py:1274: return [output_api.PresubmitError('Use C++11\'s |override| and' Weird alignment, either all align to "(" or align at +4 (my preference). https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1275 PRESUBMIT.py:1275: ' |final| rather than OVERRIDE and FINAL. \n' + Use '%s' instead of '+'
Dear Mr. Ruel and M. Mike, thanks for your valuable feedback. I have incorporated the changes suggested and add the unittest in PRESUBMIT_test.py file to catch the appropriate errors for respective file types supported. PTAL. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode12 PRESUBMIT.py:12: import re On 2014/10/14 14:38:20, M-A Ruel wrote: > I know it's not your fault but it's really time to remove this. Please move to > inside GetPreferredTryMasters() then fix any remaining caller to use > input_api.re. Done. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode13 PRESUBMIT.py:13: import sys On 2014/10/14 14:38:20, M-A Ruel wrote: > Same. sys.executable is accessible through input_api.executable. I moved the "import sys", whereveer it required, now. Before that I have tried: use input_api.executable inplace of sys.executable, then i got following error. AttributeError: 'InputApi' object has no attribute 'executable' For sys.path I have tried following input_api.path input_api.excutable.path but none worked. I thought input_api.os_path might not be appropriate to use. Please suggest me is it fine or do i need to do in any other way. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1261 PRESUBMIT.py:1261: On 2014/10/14 14:38:20, M-A Ruel wrote: > two lines between file level symbols. Done. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1268 PRESUBMIT.py:1268: if (re.search(r"\bOVERRIDE\b", line) or On 2014/10/14 14:38:20, M-A Ruel wrote: > input_api.re.search(r"\b(FINAL|OVERRIDE)\b", line) Done. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1274 PRESUBMIT.py:1274: return [output_api.PresubmitError('Use C++11\'s |override| and' On 2014/10/14 14:38:20, M-A Ruel wrote: > Weird alignment, either all align to "(" or align at +4 (my preference). Done. https://codereview.chromium.org/653883002/diff/60001/PRESUBMIT.py#newcode1275 PRESUBMIT.py:1275: ' |final| rather than OVERRIDE and FINAL. \n' + On 2014/10/14 14:38:20, M-A Ruel wrote: > Use '%s' instead of '+' I am using displaying the errors directly now.
Looks good, final nits. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1266 PRESUBMIT.py:1266: if (f.LocalPath().endswith(('.cc', '.mm', '.cpp', '.h'))): sort the items in the tuple https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1268 PRESUBMIT.py:1268: if (input_api.re.search(r"\b(OVERRIDE|FINAL)\b", line)): FINAL|OVERRIDE And use single quotes consistently, not double quotes. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1274 PRESUBMIT.py:1274: 'rather than OVERRIDE and FINAL.' comma always at the end of the string, never at the start of a line like you did on 1275.
mkwst@chromium.org changed required reviewers: + maruel@chromium.org
LGTM % one nit, and assuming that you take care of Maruel's comments. Thank you for adding tests. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT_test.py#newcod... PRESUBMIT_test.py:422: '#DEFINE FINAL_METHOD', Nit: For completeness, please add test cases for "SOMETHINGSOMETHING_OVERRIDE" and "SOMETHING_OVERRIDE_SOMETHING".
Thanks Mr. Ruel and Mr. Mike for review and for your time. I have incorporated the changes in Patch-set6. PTAL. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1266 PRESUBMIT.py:1266: if (f.LocalPath().endswith(('.cc', '.mm', '.cpp', '.h'))): On 2014/10/15 12:57:03, M-A Ruel wrote: > sort the items in the tuple Done. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1268 PRESUBMIT.py:1268: if (input_api.re.search(r"\b(OVERRIDE|FINAL)\b", line)): On 2014/10/15 12:57:03, M-A Ruel wrote: > FINAL|OVERRIDE > > And use single quotes consistently, not double quotes. Done. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1274 PRESUBMIT.py:1274: 'rather than OVERRIDE and FINAL.' On 2014/10/15 12:57:03, M-A Ruel wrote: > comma always at the end of the string, never at the start of a line like you did > on 1275. Done. https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT_test.py#newcod... PRESUBMIT_test.py:422: '#DEFINE FINAL_METHOD', On 2014/10/15 13:10:03, Mike West wrote: > Nit: For completeness, please add test cases for "SOMETHINGSOMETHING_OVERRIDE" > and "SOMETHING_OVERRIDE_SOMETHING". Done.
https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/80001/PRESUBMIT.py#newcode1268 PRESUBMIT.py:1268: if (input_api.re.search(r"\b(OVERRIDE|FINAL)\b", line)): On 2014/10/15 13:34:46, MRV wrote: > On 2014/10/15 12:57:03, M-A Ruel wrote: > > FINAL|OVERRIDE > > > > And use single quotes consistently, not double quotes. > > Done. I meant r'\b(FINAL|OVERRIDE)\b' in the same order I listed the last 2 times. I like when everything is sorted, it makes maintenance tasks (addition or removal of elements) much simpler. https://codereview.chromium.org/653883002/diff/100001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/100001/PRESUBMIT.py#newcode570 PRESUBMIT.py:570: args = [sys.executable, 'tools/checkperms/checkperms.py', '--root', Use input_api.python_executable, remove import sys at the top.
Dear Mr. Ruel Thanks for your precious reviews. I have updated the changes as per your feedback. PTAL. https://codereview.chromium.org/653883002/diff/100001/PRESUBMIT.py File PRESUBMIT.py (right): https://codereview.chromium.org/653883002/diff/100001/PRESUBMIT.py#newcode570 PRESUBMIT.py:570: args = [sys.executable, 'tools/checkperms/checkperms.py', '--root', On 2014/10/15 13:59:22, M-A Ruel wrote: > Use input_api.python_executable, remove import sys at the top. Done.
Last nit. https://codereview.chromium.org/653883002/diff/120001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/120001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:441: mock_file_cpp= MockFile('something.cpp', lines_h) mock_file_cpp = MockFile('something.cpp', lines_h) (same at line 451.
Dear mr.Ruel for feedback. Updated changes in new patch set. PTAL. https://codereview.chromium.org/653883002/diff/120001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/120001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:441: mock_file_cpp= MockFile('something.cpp', lines_h) On 2014/10/15 14:27:39, M-A Ruel wrote: > mock_file_cpp = MockFile('something.cpp', lines_h) > > (same at line 451. Done.
https://codereview.chromium.org/653883002/diff/140001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/140001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:440: lines_cpp = ['foo2() FINAL;'] Please fix spacing everywhere to be consistent. You changed lines_cc to lines_cpp but didn't change what I asked you to.
Dear mr.Ruel, nits were addressed. PTAL https://codereview.chromium.org/653883002/diff/140001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/140001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:440: lines_cpp = ['foo2() FINAL;'] SOn 2014/10/15 15:22:19, M-A Ruel wrote: > Please fix spacing everywhere to be consistent. You changed lines_cc to > lines_cpp but didn't change what I asked you to. Done. I misunderstood your comment, in previous patch-set, I had something.cpp with lines_h and something.h with lines_cc, i have corrected those. In new patch-set i have taken spaces issues.
https://codereview.chromium.org/653883002/diff/160001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/160001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:440: lines_cpp = ['foo2() FINAL;'] spacing is still inconsistent.
Dear Mr, Ruel, incoporated all the changes now. PTAL https://codereview.chromium.org/653883002/diff/160001/PRESUBMIT_test.py File PRESUBMIT_test.py (right): https://codereview.chromium.org/653883002/diff/160001/PRESUBMIT_test.py#newco... PRESUBMIT_test.py:440: lines_cpp = ['foo2() FINAL;'] On 2014/10/15 17:07:37, M-A Ruel wrote: > spacing is still inconsistent. Done.
lgtm, thanks
The CQ bit was checked by maruel@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/653883002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f21db965b70b61dea512fc6e98aad6e919274610 Cr-Commit-Position: refs/heads/master@{#299880}
Message was sent while issue was closed.
On 2014/10/16 12:05:59, M-A Ruel wrote: > lgtm, thanks Thanks a lot Mr. Ruel, I have learnt a lot with this patch. Thanks again for sparing your time for review. |
