[webkit-reviews] review granted: [Bug 113454] GCC 4.8 error - C++ nested class inheriting enclosing class : [Attachment 195676] patch - 5

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Han Shen
<shenhan at google.com>'s request for review:
Bug 113454: GCC 4.8 error - C++ nested class inheriting enclosing class
https://bugs.webkit.org/show_bug.cgi?id=113454

Attachment 195676: patch - 5
https://bugs.webkit.org/attachment.cgi?id=195676&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=195676&action=review


> Source/WTF/ChangeLog:38
> +	   Original code has nested classes that have enclosing class as their
parent, illustrated below -
> +	     template class <typename T>
> +	     class HashMap {
> +	       ... ...
> +	       ... ...
> +	     private:
> +	       class HashMapKeysProxy;
> +
> +	       // ERROR - nested class inherits enclosing class.
> +	       class HashMapKeysProxy : private HashMap {
> +	       };
> +	       ... ...
> +	       ... ...
> +	     };
> +
> +	   Fixed as below:
> +	     template class <typename T>
> +	     class HashMap {
> +	       ... ...
> +	       ... ...
> +	     private:
> +	       class HashMapKeysProxy;
> +
> +	       ... ...
> +	       ... ...
> +	     };
> +
> +	     template <typename T>
> +	     class HashMap<T>::HashMapKeysProxy : private HashMap<T> {
> +	       ... ...
> +	     };

The patch is great but I am still not happy about the ChangeLog.

What you need is explain what, why and how.

Basically explain: the previous code does not build on GCC 4.8. The reason is
HashMapKeysProxy and HashMapValuesProxy are defined as nested class inside
HashMap, which is illegal (you can like to a spec if possible). The solution
here is to move the said definitions outside of HashMap.

> Source/WTF/wtf/HashMap.h:264
> -	   m_impl.swap(other.m_impl); 
> +	   m_impl.swap(other.m_impl);
>      }
>  
>      template<typename T, typename U, typename V, typename W, typename X>
>      inline int HashMap<T, U, V, W, X>::size() const
>      {
> -	   return m_impl.size(); 
> +	   return m_impl.size();
>      }
>  
>      template<typename T, typename U, typename V, typename W, typename X>
>      inline int HashMap<T, U, V, W, X>::capacity() const
> -    { 
> -	   return m_impl.capacity(); 
> +    {
> +	   return m_impl.capacity();
>      }

Better leave those unchanged, it is unrelated with your fix.