Bug 113454 - GCC 4.8 error - C++ nested class inheriting enclosing class
Summary: GCC 4.8 error - C++ nested class inheriting enclosing class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-27 16:07 PDT by Han Shen
Modified: 2013-04-01 11:23 PDT (History)
6 users (show)

See Also:


Attachments
The is a patch. (4.84 KB, patch)
2013-03-27 16:07 PDT, Han Shen
benjamin: review-
Details | Formatted Diff | Diff
Patch again (5.32 KB, patch)
2013-03-27 16:35 PDT, Han Shen
benjamin: review-
Details | Formatted Diff | Diff
Patch - 3 (4.60 KB, patch)
2013-03-28 10:23 PDT, Han Shen
benjamin: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch - 4 (5.31 KB, patch)
2013-03-28 16:06 PDT, Han Shen
no flags Details | Formatted Diff | Diff
patch - 5 (6.51 KB, patch)
2013-03-28 16:32 PDT, Han Shen
benjamin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
patch - 6 (6.04 KB, patch)
2013-04-01 10:14 PDT, Han Shen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Han Shen 2013-03-27 16:07:11 PDT
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.
Comment 1 Benjamin Poulain 2013-03-27 16:25:31 PDT
Comment on attachment 195425 [details]
The is a patch.

You forgot the ChangeLog.
Comment 2 Han Shen 2013-03-27 16:35:43 PDT
Created attachment 195431 [details]
Patch again
Comment 3 Benjamin Poulain 2013-03-27 16:41:15 PDT
Comment on attachment 195431 [details]
Patch again

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

> ChangeLog:6
> +2013-03-27  Han Shen  <shenhan@google.com>
> +
> +        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.
Comment 4 Han Shen 2013-03-28 10:23:35 PDT
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.)
Comment 5 Benjamin Poulain 2013-03-28 10:42:34 PDT
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.
Comment 6 Build Bot 2013-03-28 11:24:05 PDT
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
Comment 7 Benjamin Poulain 2013-03-28 12:20:20 PDT
Comment on attachment 195591 [details]
Patch - 3

r- per comments and bot. Please make sure this build on modern clangs.
Comment 8 Han Shen 2013-03-28 16:06:17 PDT
Created attachment 195666 [details]
patch - 4

Thanks!

Fixed. Built (under linux) successfully using gcc 4.7/gcc 4.8/clang 3.x.
Comment 9 WebKit Review Bot 2013-03-28 16:11:08 PDT
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.
Comment 10 Han Shen 2013-03-28 16:32:27 PDT
Created attachment 195676 [details]
patch - 5

Passed "Tools/Scripts/check-webkit-style".
Comment 11 Build Bot 2013-03-28 18:45:31 PDT
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
Comment 12 Benjamin Poulain 2013-03-28 20:27:54 PDT
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.
Comment 13 Han Shen 2013-04-01 10:14:35 PDT
Created attachment 195986 [details]
patch - 6

Hi Benjamin, thanks!

ChangeLog refined and unrelated modification reverted.

Thanks,
-Han
Comment 14 Benjamin Poulain 2013-04-01 10:54:49 PDT
Comment on attachment 195986 [details]
patch - 6

Great. Thank you for all the updates.
Comment 15 WebKit Review Bot 2013-04-01 11:22:56 PDT
Comment on attachment 195986 [details]
patch - 6

Clearing flags on attachment: 195986

Committed r147345: <http://trac.webkit.org/changeset/147345>
Comment 16 WebKit Review Bot 2013-04-01 11:23:00 PDT
All reviewed patches have been landed.  Closing bug.