Issue 5712621990838272: [IE] Replace ATL::CSimpleArray (Closed)
DescriptionReplace ATL:CSimpleArray with std::set. This eliminates CSimpleArray
Patch Set 1 #
Total comments: 9
Patch Set 2 : Formatting changes #Patch Set 3 : full patch #
Total comments: 2
Patch Set 4 : #
MessagesTotal messages: 12
The first string of the description is neither according to the format "Issue 123 - Should do foo, not bar" nor "Noissue - Should do foo, not bar". The rest LGTM.
http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:1065: CPluginClass* return_instance = NULL; According to the coding style it should be `nullptr` instead of `NULL`
http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:264: s_instances.insert( this ); Style nit: no space after '(' and before ')' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:375: if ( s_instances.empty() ) Style nit: no space after '(' and before ')' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:896: for ( auto instance : s_instances ) Style nit: no space after '(' and before ')' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:898: if ( instance->m_hTabWnd == hTabWnd2) Style nit: no space after '(' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:1069: for ( auto instance : s_instances ) Style nit: no space after '(' and before ')' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:1071: if ( instance->m_hStatusBarWnd == hStatusBarWnd) Style nit: no space after '(' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:1952: for ( auto instance : s_instances ) Style nit: no space after '(' and before ')' http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... src/plugin/PluginClass.cpp:1954: if ( instance->m_hTabWnd == hTabWnd2 ) Style nit: no space after '(' and before ')'
On 2014/07/21 09:00:54, Oleksandr wrote: > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > File src/plugin/PluginClass.cpp (right): > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:264: s_instances.insert( this ); > Style nit: no space after '(' and before ')' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:375: if ( s_instances.empty() ) > Style nit: no space after '(' and before ')' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:896: for ( auto instance : s_instances ) > Style nit: no space after '(' and before ')' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:898: if ( instance->m_hTabWnd == hTabWnd2) > Style nit: no space after '(' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:1069: for ( auto instance : s_instances ) > Style nit: no space after '(' and before ')' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:1071: if ( instance->m_hStatusBarWnd == > hStatusBarWnd) > Style nit: no space after '(' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:1952: for ( auto instance : s_instances ) > Style nit: no space after '(' and before ')' > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > src/plugin/PluginClass.cpp:1954: if ( instance->m_hTabWnd == hTabWnd2 ) > Style nit: no space after '(' and before ')' I also prefer without such spaces, it would be good to put it into the coding style.
On 2014/07/21 09:41:53, sergei wrote: > On 2014/07/21 09:00:54, Oleksandr wrote: > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > File src/plugin/PluginClass.cpp (right): > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:264: s_instances.insert( this ); > > Style nit: no space after '(' and before ')' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:375: if ( s_instances.empty() ) > > Style nit: no space after '(' and before ')' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:896: for ( auto instance : s_instances ) > > Style nit: no space after '(' and before ')' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:898: if ( instance->m_hTabWnd == hTabWnd2) > > Style nit: no space after '(' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:1069: for ( auto instance : s_instances ) > > Style nit: no space after '(' and before ')' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:1071: if ( instance->m_hStatusBarWnd == > > hStatusBarWnd) > > Style nit: no space after '(' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:1952: for ( auto instance : s_instances ) > > Style nit: no space after '(' and before ')' > > > > > http://codereview.adblockplus.org/5712621990838272/diff/5629499534213120/src/... > > src/plugin/PluginClass.cpp:1954: if ( instance->m_hTabWnd == hTabWnd2 ) > > Style nit: no space after '(' and before ')' > > I also prefer without such spaces, it would be good to put it into the coding > style. It's in the Mozilla Coding Style, and also covered by the catch-all consistency rule (code should be consistent with the surrounding code, code in the same module, project etc.)
New patch set.
Just, for the future, as I understood, each patch set should be the full ready to apply patch. This tool allows to view the diff between them. As well pay attention to the "Delta from patch set" column, as well when you view side by side it's possible to view the diff between patch sets (controller in the top-left corner). The example is available here http://codereview.adblockplus.org/6307944991817728/ .
Look good, only one nit below. http://codereview.adblockplus.org/5712621990838272/diff/5733935958982656/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5712621990838272/diff/5733935958982656/src/... src/plugin/PluginClass.cpp:1064: CPluginClass* return_instance = nullptr; Nit: this variable should simply be called "result" or something like that.
http://codereview.adblockplus.org/5712621990838272/diff/5733935958982656/src/... File src/plugin/PluginClass.cpp (right): http://codereview.adblockplus.org/5712621990838272/diff/5733935958982656/src/... src/plugin/PluginClass.cpp:1064: CPluginClass* return_instance = nullptr; On 2014/07/24 06:41:53, Wladimir Palant wrote: > Nit: this variable should simply be called "result" or something like that. Done.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
