RESOLVED FIXED 113454
GCC 4.8 error - C++ nested class inheriting enclosing class
https://bugs.webkit.org/show_bug.cgi?id=113454
Summary GCC 4.8 error - C++ nested class inheriting enclosing class
Han Shen
Reported 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.
Attachments
The is a patch. (4.84 KB, patch)
2013-03-27 16:07 PDT, Han Shen
benjamin: review-
Patch again (5.32 KB, patch)
2013-03-27 16:35 PDT, Han Shen
benjamin: review-
Patch - 3 (4.60 KB, patch)
2013-03-28 10:23 PDT, Han Shen
benjamin: review-
buildbot: commit-queue-
patch - 4 (5.31 KB, patch)
2013-03-28 16:06 PDT, Han Shen
no flags
patch - 5 (6.51 KB, patch)
2013-03-28 16:32 PDT, Han Shen
benjamin: review+
buildbot: commit-queue-
patch - 6 (6.04 KB, patch)
2013-04-01 10:14 PDT, Han Shen
no flags
Benjamin Poulain
Comment 1 2013-03-27 16:25:31 PDT
Comment on attachment 195425 [details] The is a patch. You forgot the ChangeLog.
Han Shen
Comment 2 2013-03-27 16:35:43 PDT
Created attachment 195431 [details] Patch again
Benjamin Poulain
Comment 3 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.
Han Shen
Comment 4 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.)
Benjamin Poulain
Comment 5 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.
Build Bot
Comment 6 2013-03-28 11:24:05 PDT
Benjamin Poulain
Comment 7 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.
Han Shen
Comment 8 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.
WebKit Review Bot
Comment 9 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.
Han Shen
Comment 10 2013-03-28 16:32:27 PDT
Created attachment 195676 [details] patch - 5 Passed "Tools/Scripts/check-webkit-style".
Build Bot
Comment 11 2013-03-28 18:45:31 PDT
Benjamin Poulain
Comment 12 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.
Han Shen
Comment 13 2013-04-01 10:14:35 PDT
Created attachment 195986 [details] patch - 6 Hi Benjamin, thanks! ChangeLog refined and unrelated modification reverted. Thanks, -Han
Benjamin Poulain
Comment 14 2013-04-01 10:54:49 PDT
Comment on attachment 195986 [details] patch - 6 Great. Thank you for all the updates.
WebKit Review Bot
Comment 15 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>
WebKit Review Bot
Comment 16 2013-04-01 11:23:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.