Bug 29231 – Compiler warnings in InspectorResource.h

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

[Xan Lopez]

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

[Joseph Pecoraro]

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.

[Xan Lopez]

(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); }

[Joseph Pecoraro]

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.

[Martin Robinson]

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.

[Pavel Feldman]

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

[Martin Robinson]

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.

[Darin Adler]

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

[Martin Robinson]

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

[Eric Seidel (no email)]

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.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.