29231 2009-09-13 02:10:06 -0700 Compiler warnings in InspectorResource.h 2009-11-09 17:25:12 -0800 1 1 1 Unclassified WebKit Web Inspector (Deprecated) 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Normal --- 1 xan.lopez webkit-unassigned commit-queue joepeck laszlo.gombos mrobinson oldest_to_newest 146982 0 xan.lopez 2009-09-13 02:10:06 -0700 These warnings do not look so good: CXX WebCore/inspector/libWebCore_la-InspectorResource.lo ../../WebCore/inspector/InspectorResource.h: In member function ‘void WebCore::InspectorResource::updateScriptObject(WebCore::InspectorFrontend*)’: ../../WebCore/inspector/InspectorResource.h:118: warning: comparison always false due to limited range of data type ../../WebCore/inspector/InspectorResource.h:118: warning: comparison always false due to limited range of data type ../../WebCore/inspector/InspectorResource.h:118: warning: comparison always false due to limited range of data type ../../WebCore/inspector/InspectorResource.h:118: warning: comparison always false due to limited range of data type ../../WebCore/inspector/InspectorResource.h:118: warning: comparison always false due to limited range of data type ../../WebCore/inspector/InspectorResource.h:118: warning: comparison always false due to limited range of data type 150253 1 joepeck 2009-09-25 23:05:03 -0700 That is interesting. Lines "118" in InspectorResource.h don't look relevant anymore. Could you specify a revision or show the lines where the warnings are coming from? This type of error normally happens when comparing an "unsigned" with something signed. 150254 2 xan.lopez 2009-09-25 23:14:20 -0700 (In reply to comment #1) > That is interesting. Lines "118" in InspectorResource.h don't look relevant > anymore. Could you specify a revision or show the lines where the warnings are > coming from? > > This type of error normally happens when comparing an "unsigned" with something > signed. The line is: inline bool hasChange(ChangeType change) { return (m_change & change) || !(m_change + change); } 150257 3 joepeck 2009-09-25 23:26:55 -0700 As I understood it, an enum is interchangeable with an int, and those operations are fine on an int. It looks like the 6 warnings come from "InspectorResource::updateScriptObject" in InspectorResource.cpp where it calls "hasChange", which is an inline function. The calls all look just fine, passing an enum value. I'm stumped for now. 161394 4 42709 mrobinson 2009-11-07 20:17:47 -0800 Created attachment 42709 Remove addition with variables of enum type It seems the problem is the addition operation. Removing it fixing the warning for me. I've attached a patch. 161402 5 42709 pfeldman 2009-11-07 23:22:22 -0800 Comment on attachment 42709 Remove addition with variables of enum type I think that after you patch hasChange(NoChange) will always be true. Hence resources will stop updating due to the guard expression in InspectorResource::updateScriptObject. I guess what you should do is: - remove NoChange at all - introduce inline hasChanges that could return m_change != 0 - replace hasChange(NoChange) call with hasChanges - and roll back rest (non need to change numbers). 161419 6 42714 mrobinson 2009-11-08 09:59:53 -0800 Created attachment 42714 Remove addition with variables of enum type (rev2) Thanks for spotting my mistake in your review. I've attached a patch that is much less intrusive than even my first one. 161608 7 42714 darin 2009-11-09 09:42:01 -0800 Comment on attachment 42714 Remove addition with variables of enum type (rev2) > + (WebCore::InspectorResource::): The above line is due to a bug in prepare-ChangeLog. It would be better to delete a line like that if you see it in your change log. The prepare-ChangeLog tool is there to help you write a log message, but you need to read and edit it before submitting it. > - inline bool hasChange(ChangeType change) { return (m_change & change) || !(m_change + change); } > + inline bool hasChange(ChangeType change) > + { > + return m_change & change || (m_change == NoChange && change == NoChange); > + } > inline void set(ChangeType change) > { > - m_change = static_cast<ChangeType>(static_cast<unsigned>(m_change) | static_cast<unsigned>(change)); > + m_change = static_cast<ChangeType>(static_cast<unsigned>(m_change) | static_cast<unsigned>(change)); > } > - inline bool hasChange(ChangeType change) { return (m_change & change) || !(m_change + change); } > + inline bool hasChange(ChangeType change) > + { > + return m_change & change || (m_change == NoChange && change == NoChange); > + } > inline void set(ChangeType change) > { > - m_change = static_cast<ChangeType>(static_cast<unsigned>(m_change) | static_cast<unsigned>(change)); > + m_change = static_cast<ChangeType>(static_cast<unsigned>(m_change) | static_cast<unsigned>(change)); > } Not the patch, but the existing code has a number of minor problems. For example, there's no need for or value to specifying inline for functions defined inside a class definition, so all those inline are not needed. It's strange to do "&" on the two enum values without a typecast, but cast them to unsigned to do "|" on them. I don't get it. It also doesn't seem good to have the hasChange function do two different jobs. Having an argument of 0 be a special value meaning check if there is any change doesn't seem like good design to me. As far as the patch is concerned, it seems strange to remove the parentheses around the "&" subexpression but then add them around the new "&&" subexpression. But it does look like it will fix the bug, and it's not important to tweak these minor details. So r=me 161626 8 mrobinson 2009-11-09 10:01:57 -0800 (In reply to comment #7) > > + (WebCore::InspectorResource::): > The above line is due to a bug in prepare-ChangeLog. It would be better to > delete a line like that if you see it in your change log. The prepare-ChangeLog > tool is there to help you write a log message, but you need to read and edit it > before submitting it. Noted for the future! > As far as the patch is concerned, it seems strange to remove the parentheses > around the "&" subexpression but then add them around the new "&&" > subexpression. An unfortunate mistake on my part. Perhaps in the future, if this code is improved, a tweak here can be in the fix. 161841 9 42714 eric 2009-11-09 17:06:50 -0800 Comment on attachment 42714 Remove addition with variables of enum type (rev2) Martin doesn't have his bit yet, so adding this to the queue. 161850 10 42714 commit-queue 2009-11-09 17:25:07 -0800 Comment on attachment 42714 Remove addition with variables of enum type (rev2) Clearing flags on attachment: 42714 Committed r50700: <http://trac.webkit.org/changeset/50700> 161851 11 commit-queue 2009-11-09 17:25:12 -0800 All reviewed patches have been landed. Closing bug. 42709 2009-11-07 20:17:47 -0800 2009-11-08 09:59:53 -0800 Remove addition with variables of enum type 0001-Fix-compilation-warnings-by-removing-addition-operat.patch text/plain 3029 mrobinson RnJvbSA4OWM0OGFlMjQzMjM1Yzc2ZWY5NGM4OWIyZGI4NDgxZGMxYzNlZDI2IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJ0aW4gUm9iaW5zb24gPG1hcnRpbi5qYW1lcy5yb2JpbnNv bkBnbWFpbC5jb20+CkRhdGU6IFNhdCwgNyBOb3YgMjAwOSAyMDoxMTo1MyAtMDgwMApTdWJqZWN0 OiBbUEFUQ0hdIEZpeCBjb21waWxhdGlvbiB3YXJuaW5ncyBieSByZW1vdmluZyBhZGRpdGlvbiBv cGVyYXRpb24gb24gYW4gZW51bSB0eXBlLgoKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAgICAgICAg ICAgICAgICAgICAgIHwgICAxNSArKysrKysrKysrKysrKysKIFdlYkNvcmUvaW5zcGVjdG9yL0lu c3BlY3RvclJlc291cmNlLmggfCAgIDIwICsrKysrKysrKystLS0tLS0tLS0tCiAyIGZpbGVzIGNo YW5nZWQsIDI1IGluc2VydGlvbnMoKyksIDEwIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1dl YkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDVlNDAyNi4uMTY2YjFj NiAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cK QEAgLTEsMyArMSwxOCBAQAorMjAwOS0xMS0wNyAgTWFydGluIFJvYmluc29uICA8bWFydGluLmph bWVzLnJvYmluc29uQGdtYWlsLmNvbT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBDb21waWxlciB3YXJuaW5ncyBpbiBJbnNwZWN0b3JSZXNvdXJjZS5o CisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yOTIzMQor CisgICAgICAgIEZpeCBjb21waWxhdGlvbiB3YXJuaW5ncyBieSByZW1vdmluZyBhZGRpdGlvbiBv cGVyYXRpb24gb24gYW4gZW51bSB0eXBlLgorCisgICAgICAgICogaW5zcGVjdG9yL0luc3BlY3Rv clJlc291cmNlLmg6CisgICAgICAgIChXZWJDb3JlOjpJbnNwZWN0b3JSZXNvdXJjZTo6KToKKyAg ICAgICAgKFdlYkNvcmU6Okluc3BlY3RvclJlc291cmNlOjpDaGFuZ2VzOjpoYXNDaGFuZ2UpOgor ICAgICAgICAoV2ViQ29yZTo6SW5zcGVjdG9yUmVzb3VyY2U6OkNoYW5nZXM6OnNldCk6CisgICAg ICAgIChXZWJDb3JlOjpJbnNwZWN0b3JSZXNvdXJjZTo6Q2hhbmdlczo6c2V0QWxsKToKKwogMjAw OS0xMS0wNyAgRGFuaWVsIEJhdGVzICA8ZGJhdGVzQHdlYmtpdC5vcmc+CiAKICAgICAgICAgUmV2 aWV3ZWQgYnkgRGFyaW4gQWRsZXIuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2luc3BlY3Rvci9JbnNw ZWN0b3JSZXNvdXJjZS5oIGIvV2ViQ29yZS9pbnNwZWN0b3IvSW5zcGVjdG9yUmVzb3VyY2UuaApp bmRleCAwMzM1NTg2Li5lZmFhNTZjIDEwMDY0NAotLS0gYS9XZWJDb3JlL2luc3BlY3Rvci9JbnNw ZWN0b3JSZXNvdXJjZS5oCisrKyBiL1dlYkNvcmUvaW5zcGVjdG9yL0luc3BlY3RvclJlc291cmNl LmgKQEAgLTExMiwzMCArMTEyLDMwIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKIAogICAgIHByaXZh dGU6CiAgICAgICAgIGVudW0gQ2hhbmdlVHlwZSB7Ci0gICAgICAgICAgICBOb0NoYW5nZSA9IDAs Ci0gICAgICAgICAgICBSZXF1ZXN0Q2hhbmdlID0gMSwKLSAgICAgICAgICAgIFJlc3BvbnNlQ2hh bmdlID0gMiwKLSAgICAgICAgICAgIFR5cGVDaGFuZ2UgPSA0LAotICAgICAgICAgICAgTGVuZ3Ro Q2hhbmdlID0gOCwKLSAgICAgICAgICAgIENvbXBsZXRpb25DaGFuZ2UgPSAxNiwKLSAgICAgICAg ICAgIFRpbWluZ0NoYW5nZSA9IDMyCisgICAgICAgICAgICBOb0NoYW5nZSA9IDEsCisgICAgICAg ICAgICBSZXF1ZXN0Q2hhbmdlID0gMiwKKyAgICAgICAgICAgIFJlc3BvbnNlQ2hhbmdlID0gNCwK KyAgICAgICAgICAgIFR5cGVDaGFuZ2UgPSA4LAorICAgICAgICAgICAgTGVuZ3RoQ2hhbmdlID0g MTYsCisgICAgICAgICAgICBDb21wbGV0aW9uQ2hhbmdlID0gMzIsCisgICAgICAgICAgICBUaW1p bmdDaGFuZ2UgPSA2NAogICAgICAgICB9OwogCiAgICAgICAgIGNsYXNzIENoYW5nZXMgewogICAg ICAgICBwdWJsaWM6CiAgICAgICAgICAgICBDaGFuZ2VzKCkgOiBtX2NoYW5nZShOb0NoYW5nZSkg e30KIAotICAgICAgICAgICAgaW5saW5lIGJvb2wgaGFzQ2hhbmdlKENoYW5nZVR5cGUgY2hhbmdl KSB7IHJldHVybiAobV9jaGFuZ2UgJiBjaGFuZ2UpIHx8ICEobV9jaGFuZ2UgKyBjaGFuZ2UpOyB9 CisgICAgICAgICAgICBpbmxpbmUgYm9vbCBoYXNDaGFuZ2UoQ2hhbmdlVHlwZSBjaGFuZ2UpIHsg cmV0dXJuIG1fY2hhbmdlICYgY2hhbmdlOyB9CiAgICAgICAgICAgICBpbmxpbmUgdm9pZCBzZXQo Q2hhbmdlVHlwZSBjaGFuZ2UpCiAgICAgICAgICAgICB7Ci0gICAgICAgICAgICAgICAgbV9jaGFu Z2UgPSBzdGF0aWNfY2FzdDxDaGFuZ2VUeXBlPihzdGF0aWNfY2FzdDx1bnNpZ25lZD4obV9jaGFu Z2UpIHwgc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KGNoYW5nZSkpOyAgICAgICAgICAgIAorICAgICAg ICAgICAgICAgIG1fY2hhbmdlID0gc3RhdGljX2Nhc3Q8Q2hhbmdlVHlwZT4oc3RhdGljX2Nhc3Q8 dW5zaWduZWQ+KG1fY2hhbmdlKSB8IHN0YXRpY19jYXN0PHVuc2lnbmVkPihjaGFuZ2UpKTsKICAg ICAgICAgICAgIH0KICAgICAgICAgICAgIGlubGluZSB2b2lkIGNsZWFyKENoYW5nZVR5cGUgY2hh bmdlKQogICAgICAgICAgICAgewogICAgICAgICAgICAgICAgIG1fY2hhbmdlID0gc3RhdGljX2Nh c3Q8Q2hhbmdlVHlwZT4oc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KG1fY2hhbmdlKSAmIH5zdGF0aWNf Y2FzdDx1bnNpZ25lZD4oY2hhbmdlKSk7CiAgICAgICAgICAgICB9CiAKLSAgICAgICAgICAgIGlu bGluZSB2b2lkIHNldEFsbCgpIHsgbV9jaGFuZ2UgPSBzdGF0aWNfY2FzdDxDaGFuZ2VUeXBlPig2 Myk7IH0KKyAgICAgICAgICAgIGlubGluZSB2b2lkIHNldEFsbCgpIHsgbV9jaGFuZ2UgPSBzdGF0 aWNfY2FzdDxDaGFuZ2VUeXBlPigxMjcpOyB9CiAgICAgICAgICAgICBpbmxpbmUgdm9pZCBjbGVh ckFsbCgpIHsgbV9jaGFuZ2UgPSBOb0NoYW5nZTsgfQogCiAgICAgICAgIHByaXZhdGU6Ci0tIAox LjYuMy4zCgo= 42714 2009-11-08 09:59:53 -0800 2009-11-09 17:25:06 -0800 Remove addition with variables of enum type (rev2) 0001-Fix-compilation-warnings-by-removing-addition-operat.patch text/plain 2210 mrobinson RnJvbSAzZDNkOWIyMmQ1ZTJjNDBmNmUwYzlkNGZmYzEwYjNmMmU2MWFmOTM4IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBNYXJ0aW4gUm9iaW5zb24gPG1hcnRpbi5qYW1lcy5yb2JpbnNv bkBnbWFpbC5jb20+CkRhdGU6IFNhdCwgNyBOb3YgMjAwOSAyMDoxMTo1MyAtMDgwMApTdWJqZWN0 OiBbUEFUQ0hdIEZpeCBjb21waWxhdGlvbiB3YXJuaW5ncyBieSByZW1vdmluZyBhZGRpdGlvbiBv cGVyYXRpb24gb24gYW4gZW51bSB0eXBlLgoKLS0tCiBXZWJDb3JlL0NoYW5nZUxvZyAgICAgICAg ICAgICAgICAgICAgIHwgICAxNSArKysrKysrKysrKysrKysKIFdlYkNvcmUvaW5zcGVjdG9yL0lu c3BlY3RvclJlc291cmNlLmggfCAgICA3ICsrKysrLS0KIDIgZmlsZXMgY2hhbmdlZCwgMjAgaW5z ZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQgYS9XZWJDb3JlL0NoYW5nZUxv ZyBiL1dlYkNvcmUvQ2hhbmdlTG9nCmluZGV4IGI3MWRhNjEuLmIyMDY4NzIgMTAwNjQ0Ci0tLSBh L1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMDktMTEtMDggIE1hcnRpbiBSb2JpbnNvbiAgPG1hcnRpbi5qYW1lcy5yb2JpbnNvbkBn bWFpbC5jb20+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgQ29tcGlsZXIgd2FybmluZ3MgaW4gSW5zcGVjdG9yUmVzb3VyY2UuaAorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjkyMzEKKworICAgICAgICBGaXgg Y29tcGlsYXRpb24gd2FybmluZ3MgYnkgcmVtb3ZpbmcgYWRkaXRpb24gb3BlcmF0aW9uIG9uIGFu IGVudW0gdHlwZS4KKworICAgICAgICAqIGluc3BlY3Rvci9JbnNwZWN0b3JSZXNvdXJjZS5oOgor ICAgICAgICAoV2ViQ29yZTo6SW5zcGVjdG9yUmVzb3VyY2U6Oik6CisgICAgICAgIChXZWJDb3Jl OjpJbnNwZWN0b3JSZXNvdXJjZTo6Q2hhbmdlczo6aGFzQ2hhbmdlKToKKyAgICAgICAgKFdlYkNv cmU6Okluc3BlY3RvclJlc291cmNlOjpDaGFuZ2VzOjpzZXQpOgorICAgICAgICAoV2ViQ29yZTo6 SW5zcGVjdG9yUmVzb3VyY2U6OkNoYW5nZXM6OnNldEFsbCk6CisKIDIwMDktMTEtMDggIERhbiBC ZXJuc3RlaW4gIDxtaXR6QGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBTaW1vbiBG cmFzZXIuCmRpZmYgLS1naXQgYS9XZWJDb3JlL2luc3BlY3Rvci9JbnNwZWN0b3JSZXNvdXJjZS5o IGIvV2ViQ29yZS9pbnNwZWN0b3IvSW5zcGVjdG9yUmVzb3VyY2UuaAppbmRleCAwMzM1NTg2Li5i NzkyMTVjIDEwMDY0NAotLS0gYS9XZWJDb3JlL2luc3BlY3Rvci9JbnNwZWN0b3JSZXNvdXJjZS5o CisrKyBiL1dlYkNvcmUvaW5zcGVjdG9yL0luc3BlY3RvclJlc291cmNlLmgKQEAgLTEyNSwxMCAr MTI1LDEzIEBAIG5hbWVzcGFjZSBXZWJDb3JlIHsKICAgICAgICAgcHVibGljOgogICAgICAgICAg ICAgQ2hhbmdlcygpIDogbV9jaGFuZ2UoTm9DaGFuZ2UpIHt9CiAKLSAgICAgICAgICAgIGlubGlu ZSBib29sIGhhc0NoYW5nZShDaGFuZ2VUeXBlIGNoYW5nZSkgeyByZXR1cm4gKG1fY2hhbmdlICYg Y2hhbmdlKSB8fCAhKG1fY2hhbmdlICsgY2hhbmdlKTsgfQorICAgICAgICAgICAgaW5saW5lIGJv b2wgaGFzQ2hhbmdlKENoYW5nZVR5cGUgY2hhbmdlKQorICAgICAgICAgICAgeworICAgICAgICAg ICAgICAgIHJldHVybiBtX2NoYW5nZSAmIGNoYW5nZSB8fCAobV9jaGFuZ2UgPT0gTm9DaGFuZ2Ug JiYgY2hhbmdlID09IE5vQ2hhbmdlKTsKKyAgICAgICAgICAgIH0KICAgICAgICAgICAgIGlubGlu ZSB2b2lkIHNldChDaGFuZ2VUeXBlIGNoYW5nZSkKICAgICAgICAgICAgIHsKLSAgICAgICAgICAg ICAgICBtX2NoYW5nZSA9IHN0YXRpY19jYXN0PENoYW5nZVR5cGU+KHN0YXRpY19jYXN0PHVuc2ln bmVkPihtX2NoYW5nZSkgfCBzdGF0aWNfY2FzdDx1bnNpZ25lZD4oY2hhbmdlKSk7ICAgICAgICAg ICAgCisgICAgICAgICAgICAgICAgbV9jaGFuZ2UgPSBzdGF0aWNfY2FzdDxDaGFuZ2VUeXBlPihz dGF0aWNfY2FzdDx1bnNpZ25lZD4obV9jaGFuZ2UpIHwgc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KGNo YW5nZSkpOwogICAgICAgICAgICAgfQogICAgICAgICAgICAgaW5saW5lIHZvaWQgY2xlYXIoQ2hh bmdlVHlwZSBjaGFuZ2UpCiAgICAgICAgICAgICB7Ci0tIAoxLjYuMy4zCgo=