Bug 113454 – GCC 4.8 error - C++ nested class inheriting enclosing class

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Han Shen]

Created attachment 195425 [details]
The is a patch.

In WebKit/Source/WTF/wtf/HashMap.h, HashMapKeysProxy and HashMapValuesProxy are defined as nested class inside HashMap, meanwhile they inherit the enclosing HashMap class, which by any means is not legal. 

GCC 4.8 complains about this.

So the original code is like - 

template class <typename T>
class HashMap {
  ... ...
  ... ...
private:
  class HashMapKeysProxy;

  class HashMapKeysProxy : private HashMap {
  };
  ... ...
  ... ...
};

My fix is to change the above to:
template class <typename T>
class HashMap {
  ... ...
  ... ...
private:
  class HashMapKeysProxy;

  ... ...
  ... ...
};

template <typename T>
class HashMap<T>::HashMapKeysProxy : private HashMap<T> {
  ... ...
};


The patch is attached.

[Benjamin Poulain]

Comment on attachment 195425 [details]
The is a patch.

You forgot the ChangeLog.

[Han Shen]

Created attachment 195431 [details]
Patch again

[Benjamin Poulain]

Comment on attachment 195431 [details]
Patch again

View in context: https://bugs.webkit.org/attachment.cgi?id=195431&action=review

> ChangeLog:6
> [phone]  Han Shen  <[e-mail]>
> +
> +        Move the definition of nested class that inherits enclosing class outside class definition.
> +
> +        * Source/WTF/wtf/HashMap.h: Move outside nested class definition from enclosing class.
> +

First patch I guess :)

There is a tool to generate the changelogs. Just run "./Tools/Scripts/prepare-ChangeLog --bug 113454"
The ChangeLogs have a certain standard format.

You will also need to include a description. Something similar to what you explains in the bug.

> Source/WTF/wtf/HashMap.h:147
> +            typedef typename HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg>::iterator::Keys iterator;

Please privately typedef HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg> to avoid the repeated args on every calls.

> Source/WTF/wtf/HashMap.h:186
> -            typedef typename HashMap::iterator::Values iterator;
> -            typedef typename HashMap::const_iterator::Values const_iterator;
> -            
> +            typedef typename HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg>::iterator::Values iterator;
> +            typedef typename HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg>::const_iterator::Values const_iterator;

Ditto.

[Han Shen]

Created attachment 195591 [details]
Patch - 3

Thanks Benjamin.

Done.

(Actually, it's my second patch, the ChangeLog for the previous one was done by the one who reviewed my code.)

[Benjamin Poulain]

Comment on attachment 195591 [details]
Patch - 3

View in context: https://bugs.webkit.org/attachment.cgi?id=195591&action=review

> Source/WTF/ChangeLog:9
> +
> +        * wtf/HashMap.h:
> +        (HashMap):

Here you should include a short description of your change.
You need to say what you fix (or why do you make the change), and how you fix it. (here is an example: http://trac.webkit.org/changeset/146372/trunk/Source/WTF/ChangeLog).

This description is intended for people reading your code in the future (typically a few years from now with WTF). It explains why a certain change was made.

> Source/WTF/wtf/HashMap.h:143
> +    template<typename KeyArg, typename MappedArg, typename HashArg = typename DefaultHash<KeyArg>::Hash,
> +        typename KeyTraitsArg = HashTraits<KeyArg>, typename MappedTraitsArg = HashTraits<MappedArg> >

It looks from the bot that passing default template arguments is not correct here.

> Source/WTF/wtf/HashMap.h:186
> +            typedef HashMap<KeyArg, MappedArg, HashArg, KeyTraitsArg, MappedTraitsArg> HashMapInst;

HashMapInst -> HashMapType.

[Build Bot]

Comment on attachment 195591 [details]
Patch - 3

Attachment 195591 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17306493

[Benjamin Poulain]

Comment on attachment 195591 [details]
Patch - 3

r- per comments and bot. Please make sure this build on modern clangs.

[Han Shen]

Created attachment 195666 [details]
patch - 4

Thanks!

Fixed. Built (under linux) successfully using gcc 4.7/gcc 4.8/clang 3.x.

[WebKit Review Bot]

Attachment 195666 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/HashMap.h']" exit_code: 1
Source/WTF/wtf/HashMap.h:142:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Han Shen]

Created attachment 195676 [details]
patch - 5

Passed "Tools/Scripts/check-webkit-style".

[Build Bot]

Comment on attachment 195676 [details]
patch - 5

Attachment 195676 [details] did not pass win-ews (win):
Output: http://webkit-commit-queue.appspot.com/results/17305396

[Benjamin Poulain]

Comment on attachment 195676 [details]
patch - 5

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.

[Han Shen]

Created attachment 195986 [details]
patch - 6

Hi Benjamin, thanks!

ChangeLog refined and unrelated modification reverted.

Thanks,
-Han

[Benjamin Poulain]

Comment on attachment 195986 [details]
patch - 6

Great. Thank you for all the updates.

[WebKit Review Bot]

Comment on attachment 195986 [details]
patch - 6

Clearing flags on attachment: 195986

Committed r147345: <http://trac.webkit.org/changeset/147345>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.