Author Topic: ToDo plugin patch doing some refactoring  (Read 4399 times)

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
ToDo plugin patch doing some refactoring
« on: November 20, 2015, 09:46:46 am »
I had some no-quality spare time sitting in a train and looked over one of the classes in the plugin.

only refactoring, functionality should be identical.

changes to addtododlg.h :

- changed the constructor: now takes const& instead of copying arguments
- made the destructor virtual
- removed two not necessary members
- some clean up

changes to addtododlg.cpp :

- changed the constructor: now takes const& instead of copying arguments
- moved empty destructor to header file
- removed two not necessary members
- removed some loops to add entries to wxWidgets gui elements which support adding an array of entries
- removed some loops getting the entries from gui elements, when they support retrieving an array
- some clean up

regards
« Last Edit: November 22, 2015, 09:33:32 am by frithjofh »
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #1 on: November 22, 2015, 09:33:58 am »
made some changes to the original post to include the changes made
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 9800
Re: ToDo plugin patch doing some refactoring
« Reply #2 on: November 23, 2015, 02:10:16 am »
Please minimize the number of topics you post, especially when you post patches for a single plugin...
<debugger plugin maintainer>
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #3 on: November 23, 2015, 09:54:27 am »
understood ;) - will do

will you be testing these patches?
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline oBFusCATed

  • Developer
  • Lives here!
  • *****
  • Posts: 9800
Re: ToDo plugin patch doing some refactoring
« Reply #4 on: November 23, 2015, 04:19:03 pm »
Won't have time in the next two weeks.
<debugger plugin maintainer>
(most of the time I ignore long posts)
[strangers don't send me private messages, I'll ignore them; post a topic in the forum, but first read the rules!]

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #5 on: November 30, 2015, 12:34:52 pm »
looked through the todolistview.h and todolistview.cpp

attached patch:

- removed two functions from header and made them non member functions to clean up the interface
- refactored the line counting function to reflect more transparently what it is doing and how it is used
- refactored some functions to take their arguments as const&
- added const to some member fucnctions
- refactored some functions to use for loops instead of while loops
- for consistency changed post-increment to increment where possible
- refactored the parsebuffer function:
   - added const where ever possible
   - simplified some code using already existent functions
   - replaced << with += on strings where ever possible
   - replaced wxString and wxArrayString functions with their stl counterparts admitted by wxWidgets. makes code clearer and makes better use of highlighting
   - made a string static

regards
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 4890
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #6 on: December 05, 2015, 03:12:43 pm »
I had some no-quality spare time sitting in a train and looked over one of the classes in the plugin.

only refactoring, functionality should be identical.

changes to addtododlg.h :

- changed the constructor: now takes const& instead of copying arguments
- made the destructor virtual
- removed two not necessary members
- some clean up

changes to addtododlg.cpp :

- changed the constructor: now takes const& instead of copying arguments
- moved empty destructor to header file
- removed two not necessary members
- removed some loops to add entries to wxWidgets gui elements which support adding an array of entries
- removed some loops getting the entries from gui elements, when they support retrieving an array
- some clean up

regards
First, thanks for the contribution.
I just review the patch, and I have two questions.
1, I see you have such change:
Code: [Select]
-    if (cmb->GetCount()>0)
-      cmb->SetSelection(0);
+    cmb->SetSelection(0);
It is the same thing?

2, I see that you have removed the two member variables:
Code: [Select]
-        wxArrayString m_Users;
-        wxArrayString m_Types;
         std::bitset<(int)tdctError+1> m_supportedTdcts;
         DECLARE_EVENT_TABLE()

When I read the code, I see that inside the function:
Code: [Select]
void AddTodoDlg::EndModal(int retVal)
 {
     if (retVal == wxID_OK)
     {
-        // "save" users
         wxChoice* cmb = XRCCTRL(*this, "chcUser", wxChoice);
-        m_Users.Clear();
-        if (cmb->FindString(cmb->GetStringSelection(), true) == wxNOT_FOUND)
-            m_Users.Add(cmb->GetStringSelection());
-        for (unsigned int i = 0; i < cmb->GetCount(); ++i)
-            m_Users.Add(cmb->GetString(i));
-        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("users"), m_Users);
+        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("users"), cmb->GetStrings());
         Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_user"), cmb->GetStringSelection());
 
-        // "save" types
         cmb = XRCCTRL(*this, "chcType", wxChoice);
-        m_Types.Clear();
-        if (cmb->FindString(cmb->GetStringSelection(), true) == wxNOT_FOUND)
-            m_Types.Add(cmb->GetStringSelection());
-        for (unsigned int i = 0; i < cmb->GetCount(); ++i)
-            m_Types.Add(cmb->GetString(i));
-        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("types"), m_Types);
+        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("types"), cmb->GetStrings());
         Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_type"), cmb->GetStringSelection());
 
         cmb = XRCCTRL(*this, "chcStyle", wxChoice);
         Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_style"), cmb->GetStringSelection());
Here, you just save the types and users from the GUI (through cmb->GetStrings()) to the configure files.

But when I looked at the usage of the dialog:
Code: [Select]
void ToDoList::OnAddItem(cb_unused wxCommandEvent& event)
{
    cbEditor* ed = Manager::Get()->GetEditorManager()->GetBuiltinActiveEditor();
    if (!ed)
        return;

    EditorColourSet* colour_set = Manager::Get()->GetEditorManager()->GetColourSet();
    if (!colour_set)
        return;

    HighlightLanguage hlang = colour_set->GetLanguageName(ed->GetLanguage());
    bool edIsCCpp = (hlang == _T("C/C++"));

    CommentToken token = colour_set->GetCommentToken(ed->GetLanguage());
    bool hasStreamComments = not token.streamCommentStart.IsEmpty();
    bool hasLineComments = not token.lineComment.IsEmpty();

    if (!(edIsCCpp||hasLineComments||hasStreamComments))
        return;
    std::bitset<(int)tdctError+1> supportedTdcts;
    supportedTdcts[tdctLine] = !(token.lineComment.IsEmpty());
    supportedTdcts[tdctStream] = !(token.streamCommentStart.IsEmpty());
    supportedTdcts[tdctDoxygenLine] = !(token.doxygenLineComment.IsEmpty());
    supportedTdcts[tdctDoxygenStream] = !(token.doxygenStreamCommentStart.IsEmpty());
    supportedTdcts[tdctWarning] = edIsCCpp;
    supportedTdcts[tdctError] = edIsCCpp;
    // display todo dialog
    AddTodoDlg dlg(Manager::Get()->GetAppWindow(), m_Users, m_Types, supportedTdcts );
    PlaceWindow(&dlg);
    if (dlg.ShowModal() != wxID_OK)
        return;
    // Re-load users and types as they might have changed by the user via AddTodoDlg
    LoadUsers();
    LoadTypes();

Here, in the LoadUsers(); and LoadTypes(); function, it was saved again in the configure file.
My question is: why do we need to pass the "m_Users, m_Types" as an argument to the AddTodoDlg's constructor, it looks like in the ToDoList class, we don't need the two "m_Users, m_Types" member variables either.
Inside the constructor of AddTodoDlg, it also contains some code like:
Code: [Select]
    if (users.empty())
        cmb->Append(wxGetUserId());
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 4890
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #7 on: December 05, 2015, 03:39:16 pm »
looked through the todolistview.h and todolistview.cpp

attached patch:

- removed two functions from header and made them non member functions to clean up the interface
- refactored the line counting function to reflect more transparently what it is doing and how it is used
- refactored some functions to take their arguments as const&
- added const to some member fucnctions
- refactored some functions to use for loops instead of while loops
- for consistency changed post-increment to increment where possible
- refactored the parsebuffer function:
   - added const where ever possible
   - simplified some code using already existent functions
   - replaced << with += on strings where ever possible
   - replaced wxString and wxArrayString functions with their stl counterparts admitted by wxWidgets. makes code clearer and makes better use of highlighting
   - made a string static

regards
Thanks for the contribution! I read the patch, and I see generally its OK, but one question:
What is the difference between
Code: [Select]
buffer.GetChar(pos)and
Code: [Select]
buffer[pos]If I remember correctly, in our CodeCompletion's code, it use GetChar, does it have performance issue? Since the operator return a reference?

If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #8 on: December 05, 2015, 05:51:58 pm »
@ollydbg

about the buffer.GetChar() and the buffer[] difference.

before and after the change I instrumented the code with std::chrono::high_resolution_clock to get the actual time spent in the function. this particular change didn't show any slowdown. for me it was more a matter of habit and readability. but I did not look into the generated machine code to see, if there was any actual difference. please feel free to revert to .GetChar().

and many thanks for the feedback
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #9 on: December 05, 2015, 05:59:27 pm »
Quote
1, I see you have such change:
Code: [Select]
-    if (cmb->GetCount()>0)
-      cmb->SetSelection(0);
+    cmb->SetSelection(0);
It is the same thing?

I could find none. if the numbers of items is 0, selecting the zero index element has no effect at all. the widget takes care of that by itself, no need to double-check


Quote
2, I see that you have removed the two member variables:
Code: [Select]
-        wxArrayString m_Users;
-        wxArrayString m_Types;
         std::bitset<(int)tdctError+1> m_supportedTdcts;
         DECLARE_EVENT_TABLE()

When I read the code, I see that inside the function:
Code: [Select]
void AddTodoDlg::EndModal(int retVal)
 {
     if (retVal == wxID_OK)
     {
-        // "save" users
         wxChoice* cmb = XRCCTRL(*this, "chcUser", wxChoice);
-        m_Users.Clear();
-        if (cmb->FindString(cmb->GetStringSelection(), true) == wxNOT_FOUND)
-            m_Users.Add(cmb->GetStringSelection());
-        for (unsigned int i = 0; i < cmb->GetCount(); ++i)
-            m_Users.Add(cmb->GetString(i));
-        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("users"), m_Users);
+        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("users"), cmb->GetStrings());
         Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_user"), cmb->GetStringSelection());
 
-        // "save" types
         cmb = XRCCTRL(*this, "chcType", wxChoice);
-        m_Types.Clear();
-        if (cmb->FindString(cmb->GetStringSelection(), true) == wxNOT_FOUND)
-            m_Types.Add(cmb->GetStringSelection());
-        for (unsigned int i = 0; i < cmb->GetCount(); ++i)
-            m_Types.Add(cmb->GetString(i));
-        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("types"), m_Types);
+        Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("types"), cmb->GetStrings());
         Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_type"), cmb->GetStringSelection());
 
         cmb = XRCCTRL(*this, "chcStyle", wxChoice);
         Manager::Get()->GetConfigManager(_T("todo_list"))->Write(_T("last_used_style"), cmb->GetStringSelection());
Here, you just save the types and users from the GUI (through cmb->GetStrings()) to the configure files.

But when I looked at the usage of the dialog:
Code: [Select]
void ToDoList::OnAddItem(cb_unused wxCommandEvent& event)
{
    cbEditor* ed = Manager::Get()->GetEditorManager()->GetBuiltinActiveEditor();
    if (!ed)
        return;

    EditorColourSet* colour_set = Manager::Get()->GetEditorManager()->GetColourSet();
    if (!colour_set)
        return;

    HighlightLanguage hlang = colour_set->GetLanguageName(ed->GetLanguage());
    bool edIsCCpp = (hlang == _T("C/C++"));

    CommentToken token = colour_set->GetCommentToken(ed->GetLanguage());
    bool hasStreamComments = not token.streamCommentStart.IsEmpty();
    bool hasLineComments = not token.lineComment.IsEmpty();

    if (!(edIsCCpp||hasLineComments||hasStreamComments))
        return;
    std::bitset<(int)tdctError+1> supportedTdcts;
    supportedTdcts[tdctLine] = !(token.lineComment.IsEmpty());
    supportedTdcts[tdctStream] = !(token.streamCommentStart.IsEmpty());
    supportedTdcts[tdctDoxygenLine] = !(token.doxygenLineComment.IsEmpty());
    supportedTdcts[tdctDoxygenStream] = !(token.doxygenStreamCommentStart.IsEmpty());
    supportedTdcts[tdctWarning] = edIsCCpp;
    supportedTdcts[tdctError] = edIsCCpp;
    // display todo dialog
    AddTodoDlg dlg(Manager::Get()->GetAppWindow(), m_Users, m_Types, supportedTdcts );
    PlaceWindow(&dlg);
    if (dlg.ShowModal() != wxID_OK)
        return;
    // Re-load users and types as they might have changed by the user via AddTodoDlg
    LoadUsers();
    LoadTypes();

Here, in the LoadUsers(); and LoadTypes(); function, it was saved again in the configure file.
My question is: why do we need to pass the "m_Users, m_Types" as an argument to the AddTodoDlg's constructor, it looks like in the ToDoList class, we don't need the two "m_Users, m_Types" member variables either.
Inside the constructor of AddTodoDlg, it also contains some code like:
Code: [Select]
    if (users.empty())
        cmb->Append(wxGetUserId());

I think you are right. these arguments are not really needed either. as it is right now, the plugin interchanges this state via the configmgr already. slipped my attention...
« Last Edit: December 05, 2015, 06:01:12 pm by frithjofh »
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #10 on: December 12, 2015, 05:43:22 pm »
for the speed argument between subscript and GetChar() on wxString i put this quote from wxWidgets:
Quote
Reference counting and why you shouldn't care about it

All considerations for wxObject-derived reference counted objects are valid also for wxString, even if it does not derive from wxObject.

Probably the unique case when you might want to think about reference counting is when a string character is taken from a string which is not a constant (or a constant reference). In this case, due to C++ rules, the "read-only" operator[] (which is the same as GetChar()) cannot be chosen and the "read/write" operator[] (the same as GetWritableChar()) is used instead. As the call to this operator may modify the string, its data is unshared (COW is done) and so if the string was really shared there is some performance loss (both in terms of speed and memory consumption). In the rare cases when this may be important, you might prefer using GetChar() instead of the array subscript operator for this reasons. Please note that at() method has the same problem as the subscript operator in this situation and so using it is not really better. Also note that if all string arguments to your functions are passed as const wxString& (see the section Some advice) this situation will almost never arise because for constant references the correct operator is called automatically.

as buffer is a const wxString& there should be no difference. But I will do a measurement after Christmas or when ever I will have time...
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 4890
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: ToDo plugin patch doing some refactoring
« Reply #11 on: December 13, 2015, 06:22:49 am »
Thanks for the clarification.
I think I will handle those patches after our 15.12 release. You can just ping on this thread if I forget about it.  :)
If some piece of memory should be reused, turn them to variables (or const variables).
If some piece of operations should be reused, turn them to functions.
If they happened together, then turn them to classes.

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9415
Re: ToDo plugin patch doing some refactoring
« Reply #12 on: December 13, 2015, 06:58:12 am »
What is the difference between
Code: [Select]
buffer.GetChar(pos)and
Code: [Select]
buffer[pos]If I remember correctly, in our CodeCompletion's code, it use GetChar, does it have performance issue? Since the operator return a reference?
IMHO the difference is that the first one is checked, the other one not and therefore a potential crash candidate.
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ

Offline frithjofh

  • Regular
  • ***
  • Posts: 356
Re: ToDo plugin patch doing some refactoring
« Reply #13 on: December 13, 2015, 06:36:52 pm »
if GetChar() is checked, then it must be slower than subscript...

What would be the right decision? Crash candidate or speed? Using subscript the function surely could be written so, that pos must always be a valid index into buffer, with only one check for validity some place...

Is GetChar() really checked? I couldn’t find anything about it (that means, I may just not know, where to look...)
« Last Edit: December 13, 2015, 06:53:21 pm by frithjofh »
architect with some spare time  -  c::b compiled from last svn  -   openSuSE leap x86_64  -  AMD FX-4100

Offline MortenMacFly

  • Administrator
  • Lives here!
  • *****
  • Posts: 9415
Re: ToDo plugin patch doing some refactoring
« Reply #14 on: December 13, 2015, 07:48:06 pm »
Is GetChar() really checked? I couldn’t find anything about it (that means, I may just not know, where to look...)
Check wx/string.h:

Code: C++
  1.     wxChar  GetChar(size_t n) const
  2.       { return at(n); }
  3.  
...later on:

Code: C++
  1.   value_type at(size_type n) const
  2.     { wxASSERT_VALID_INDEX( n ); return m_pchData[n]; }
  3.  

The assert macro is probably "nothing" in release builds. But it helps debugging in debug builds.

Please use GetChar. We handle many files, make complex parsing, may encounter weird encodings - its the best you can do it such situation.
« Last Edit: December 13, 2015, 07:55:58 pm by MortenMacFly »
Compiler logging: Settings->Compiler & Debugger->tab "Other"->Compiler logging="Full command line"
C::B Manual: http://www.codeblocks.org/docs/main_codeblocks_en.html
C::B FAQ: http://wiki.codeblocks.org/index.php?title=FAQ