Bug 29231 - Compiler warnings in InspectorResource.h
Summary: Compiler warnings in InspectorResource.h
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-13 02:10 PDT by Xan Lopez
Modified: 2009-11-09 17:25 PST (History)
4 users (show)

See Also:


Attachments
Remove addition with variables of enum type (2.96 KB, patch)
2009-11-07 20:17 PST, Martin Robinson
pfeldman: review-
Details | Formatted Diff | Diff
Remove addition with variables of enum type (rev2) (2.16 KB, patch)
2009-11-08 09:59 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2009-09-13 02:10:06 PDT
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
Comment 1 Joseph Pecoraro 2009-09-25 23:05:03 PDT
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.
Comment 2 Xan Lopez 2009-09-25 23:14:20 PDT
(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); }
Comment 3 Joseph Pecoraro 2009-09-25 23:26:55 PDT
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.
Comment 4 Martin Robinson 2009-11-07 20:17:47 PST
Created attachment 42709 [details]
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.
Comment 5 Pavel Feldman 2009-11-07 23:22:22 PST
Comment on attachment 42709 [details]
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).
Comment 6 Martin Robinson 2009-11-08 09:59:53 PST
Created attachment 42714 [details]
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.
Comment 7 Darin Adler 2009-11-09 09:42:01 PST
Comment on attachment 42714 [details]
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
Comment 8 Martin Robinson 2009-11-09 10:01:57 PST
(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.
Comment 9 Eric Seidel (no email) 2009-11-09 17:06:50 PST
Comment on attachment 42714 [details]
Remove addition with variables of enum type (rev2)

Martin doesn't have his bit yet, so adding this to the queue.
Comment 10 WebKit Commit Bot 2009-11-09 17:25:07 PST
Comment on attachment 42714 [details]
Remove addition with variables of enum type (rev2)

Clearing flags on attachment: 42714

Committed r50700: <http://trac.webkit.org/changeset/50700>
Comment 11 WebKit Commit Bot 2009-11-09 17:25:12 PST
All reviewed patches have been landed.  Closing bug.