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

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Vivek Galatage]

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.

[Vivek Galatage]

Patch to follow.

[Vivek Galatage]

Created attachment 151099 [details]
Patch

[Vsevolod Vlasov]

[e-mail]
Peter might want to have a look at this.

[Peter Rybin]

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]

(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 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]

Created attachment 151156 [details]
Patch

[Peter Rybin]

(id=151156)
LGTM
What's strange is that Changelog says about 2 checks, when there is only one.

[Vivek Galatage]

(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]

Created attachment 151157 [details]
Patch

[WebKit Review Bot]

Comment on attachment 151157 [details]
Patch

Clearing flags on attachment: 151157

Committed r122059: <http://trac.webkit.org/changeset/122059>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.