| 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: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
Patch to follow. Created attachment 151099 [details]
Patch
+prybin@chromium.org Peter might want to have a look at this. 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. (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 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.
Created attachment 151156 [details]
Patch
(id=151156) LGTM What's strange is that Changelog says about 2 checks, when there is only one. (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. Created attachment 151157 [details]
Patch
Comment on attachment 151157 [details] Patch Clearing flags on attachment: 151157 Committed r122059: <http://trac.webkit.org/changeset/122059> All reviewed patches have been landed. Closing bug. |
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.