Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
899503003:
Add back detection of unreachable code (Closed)
|
Created:
5 years, 10 months ago by wychen Modified:
5 years, 10 months ago CC:
chromium-reviews, cmp-cc_chromium.org, dpranke+depot_tools_chromium.org, iannucci+depot_tools_chromium.org Base URL:
https://chromium.googlesource.com/chromium/tools/depot_tools.git@master Target Ref:
refs/heads/master Project:
tools Visibility:
Public. |
DescriptionThe original assertions trigger pylint unreachable warnings, and they are replaced by fail() calls.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=293930
Patch Set 1 #Patch Set 2 : use self.fail() instead #
Total comments: 3
Patch Set 3 : remove self-explanatory msg #Messages
Total messages: 15 (3 generated)
wychen@chromium.org changed reviewers: + tandrii@chromium.org
PTAL.
On 2015/02/03 01:41:48, Wei-Yin Chen wrote: > PTAL. Thanks for the CL but it was already "fixed" with https://codereview.chromium.org/884243006/ so just sync. :) In practice the right fix would have been to use self.fail() but oh well.
Thanks for your suggestion. I was blocked by the presubmit lint warning, and then saw your version of fix when rebasing my patch. Bad timing. :p The coverage would be slightly lower when just removing these assertions, right? Made a new patch using self.fail().
maruel@chromium.org changed reviewers: + maruel@chromium.org
https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py File tests/gclient_test.py (right): https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py#ne... tests/gclient_test.py:997: self.fail("unreachable code") self.fail() is fine, it's unreachable code by definition.
I've rewritten the description to reflect the current situation. PTAL.
https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py File tests/gclient_test.py (right): https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py#ne... tests/gclient_test.py:997: self.fail("unreachable code") On 2015/02/03 02:08:06, M-A Ruel wrote: > self.fail() is fine, it's unreachable code by definition. I meant literally self.fail(), not self.fail("unreachable code").
On 2015/02/03 20:40:51, M-A Ruel wrote: > https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py > File tests/gclient_test.py (right): > > https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py#ne... > tests/gclient_test.py:997: self.fail("unreachable code") > On 2015/02/03 02:08:06, M-A Ruel wrote: > > self.fail() is fine, it's unreachable code by definition. > > I meant literally self.fail(), not self.fail("unreachable code"). Ah, I didn't see what you meant then. Thanks for clarifying. I don't understand why self.fail() is better than self.fail("unreachable code"). If somehow the code is broken in the future and that failure is triggered, the error message will contain "unreachable code", and hence easier to understand without looking up the code. Or, is this one of the examples of adding messages/comments to self-explanatory code?
On 2015/02/03 20:55:00, Wei-Yin Chen wrote: > On 2015/02/03 20:40:51, M-A Ruel wrote: > > https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py > > File tests/gclient_test.py (right): > > > > > https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py#ne... > > tests/gclient_test.py:997: self.fail("unreachable code") > > On 2015/02/03 02:08:06, M-A Ruel wrote: > > > self.fail() is fine, it's unreachable code by definition. > > > > I meant literally self.fail(), not self.fail("unreachable code"). > > Ah, I didn't see what you meant then. Thanks for clarifying. > > I don't understand why self.fail() is better than self.fail("unreachable code"). > If somehow the code is broken in the future and that failure is triggered, the > error message will contain "unreachable code", and hence easier to understand > without looking up the code. Or, is this one of the examples of adding > messages/comments to self-explanatory code? The later, yes. :)
Got it. Fixed. https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py File tests/gclient_test.py (right): https://codereview.chromium.org/899503003/diff/20001/tests/gclient_test.py#ne... tests/gclient_test.py:997: self.fail("unreachable code") On 2015/02/03 20:40:51, M-A Ruel wrote: > On 2015/02/03 02:08:06, M-A Ruel wrote: > > self.fail() is fine, it's unreachable code by definition. > > I meant literally self.fail(), not self.fail("unreachable code"). Done.
The CQ bit was checked by maruel@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/899503003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as http://src.chromium.org/viewvc/chrome?view=rev&revision=293930 |
