Summary: | GCC 4.8 error - C++ nested class inheriting enclosing class | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Han Shen <shenhan> | ||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | benjamin, buildbot, cmarcelo, ojan.autocc, rniwa, webkit.review.bot | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | Linux | ||||||||||||||||
Attachments: |
|
Comment on attachment 195425 [details]
The is a patch.
You forgot the ChangeLog.
Created attachment 195431 [details]
Patch again
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. 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 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 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 on attachment 195591 [details]
Patch - 3
r- per comments and bot. Please make sure this build on modern clangs.
Created attachment 195666 [details]
patch - 4
Thanks!
Fixed. Built (under linux) successfully using gcc 4.7/gcc 4.8/clang 3.x.
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.
Created attachment 195676 [details]
patch - 5
Passed "Tools/Scripts/check-webkit-style".
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 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. Created attachment 195986 [details]
patch - 6
Hi Benjamin, thanks!
ChangeLog refined and unrelated modification reverted.
Thanks,
-Han
Comment on attachment 195986 [details]
patch - 6
Great. Thank you for all the updates.
Comment on attachment 195986 [details] patch - 6 Clearing flags on attachment: 195986 Committed r147345: <http://trac.webkit.org/changeset/147345> All reviewed patches have been landed. Closing bug. |
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.