Bug 90697

Summary:

Web Inspector: CodeGeneratorInspector.py should not generate statements with no effect

Product:

WebKit

Reporter:

Vivek Galatage <vivekgalatage>

Component:

Web Inspector (Deprecated)

Assignee:

Vivek Galatage <vivekgalatage>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, prybin, rik, timothy, vsevik, webkit.review.bot, yurys

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

Vivek Galatage

Reported 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.

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 attachment proposed patch, testcase, etc.

Vivek Galatage

Comment 1 2012-07-06 10:31:31 PDT

Patch to follow.

Vivek Galatage

Comment 2 2012-07-06 10:33:50 PDT

Created attachment 151099 [details] Patch

Vsevolod Vlasov

Comment 3 2012-07-06 10:35:37 PDT

+prybin@chromium.org Peter might want to have a look at this.

Peter Rybin

Comment 4 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.

Vivek Galatage

Comment 5 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?

Vsevolod Vlasov

Comment 6 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.

Vivek Galatage

Comment 7 2012-07-07 21:49:05 PDT

Created attachment 151156 [details] Patch

Peter Rybin

Comment 8 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.

Vivek Galatage

Comment 9 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.

Vivek Galatage

Comment 10 2012-07-07 22:47:36 PDT

Created attachment 151157 [details] Patch

WebKit Review Bot

Comment 11 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>

WebKit Review Bot

Comment 12 2012-07-08 02:04:27 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.