[Webkit-unassigned] [Bug 29231] Compiler warnings in InspectorResource.h

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=29231


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #42714|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2009-11-09 09:42:01 PDT ---
(From update of attachment 42714)
> +        (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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.