Bug 90697 - Web Inspector: CodeGeneratorInspector.py should not generate statements with no effect
Summary: Web Inspector: CodeGeneratorInspector.py should not generate statements with ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vivek Galatage
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-06 10:30 PDT by Vivek Galatage
Modified: 2012-07-08 02:04 PDT (History)
13 users (show)

See Also:

Attachments
Patch (3.91 KB, patch)
2012-07-06 10:33 PDT, Vivek Galatage 		no flags Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2012-07-07 21:49 PDT, Vivek Galatage 		no flags Details | Formatted Diff | Diff
Patch (1.94 KB, patch)
2012-07-07 22:47 PDT, Vivek Galatage 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Vivek Galatage 2012-07-06 10:30:37 PDT 
While generating InspectorBackendDispatcher.cpp, CodeGeneratorInspector.py generates lot of statements in the code which has no effect as -

if (!error.length()) {
}

This should be avoided.
Comment 1 Vivek Galatage 2012-07-06 10:31:31 PDT 
Patch to follow.
Comment 2 Vivek Galatage 2012-07-06 10:33:50 PDT 
Created attachment 151099 [details]
Patch
Comment 3 Vsevolod Vlasov 2012-07-06 10:35:37 PDT 
+prybin@chromium.org
Peter might want to have a look at this.
Comment 4 Peter Rybin 2012-07-06 11:38:09 PDT 
Unfortunately right now I have a temperature, cannot look carefully.

However I don't really see a reason for fixing this bug. To me it complicates generator (already too complex) without a clear benefit. I think the compiler should optimize off this statement by itself.
Comment 5 Vivek Galatage 2012-07-06 12:06:36 PDT 
(In reply to comment #4)
> Unfortunately right now I have a temperature, cannot look carefully.
> 
> However I don't really see a reason for fixing this bug. To me it complicates generator (already too complex) without a clear benefit. I think the compiler should optimize off this statement by itself.

I agree that compiler might nuke this off while optimizing. But while reading through the code, it doesn't convey any meaning. This in fact might create some confusion whether this piece of code has some incomplete implementation. I also agree that in this process, the Generator might become complex. But this complexity will add to more clear readability of the generated code. Your thoughts?
Comment 6 Vsevolod Vlasov 2012-07-06 12:14:33 PDT 
Comment on attachment 151099 [details]
Patch

This indeed looks too complex for such a small value added. r- per Peter's comments. 
Unless it is possible to make a one or two lines fix I don't think this is reasonable.
Comment 7 Vivek Galatage 2012-07-07 21:49:05 PDT 
Created attachment 151156 [details]
Patch
Comment 8 Peter Rybin 2012-07-07 22:22:01 PDT 
(id=151156)
LGTM
What's strange is that Changelog says about 2 checks, when there is only one.
Comment 9 Vivek Galatage 2012-07-07 22:42:56 PDT 
(In reply to comment #8)
> (id=151156)
> LGTM
> What's strange is that Changelog says about 2 checks, when there is only one.

:) I will correct the Changelog and re-upload the patch. Thank you for the review.
Comment 10 Vivek Galatage 2012-07-07 22:47:36 PDT 
Created attachment 151157 [details]
Patch
Comment 11 WebKit Review Bot 2012-07-08 02:04:21 PDT 
Comment on attachment 151157 [details]
Patch

Clearing flags on attachment: 151157

Committed r122059: <http://trac.webkit.org/changeset/122059>
Comment 12 WebKit Review Bot 2012-07-08 02:04:27 PDT 
All reviewed patches have been landed.  Closing bug.