Developer forums (C::B DEVELOPMENT STRICTLY!) > Plugins development
ToDo plugin patch doing some refactoring
frithjofh:
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
ollydbg:
--- Quote from: frithjofh 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
--- End quote ---
First, thanks for the contribution.
I just review the patch, and I have two questions.
1, I see you have such change:
--- Code: ---- if (cmb->GetCount()>0)
- cmb->SetSelection(0);
+ cmb->SetSelection(0);
--- End code ---
It is the same thing?
2, I see that you have removed the two member variables:
--- Code: ---- wxArrayString m_Users;
- wxArrayString m_Types;
std::bitset<(int)tdctError+1> m_supportedTdcts;
DECLARE_EVENT_TABLE()
--- End code ---
When I read the code, I see that inside the function:
--- Code: --- 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());
--- End code ---
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: ---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();
--- End code ---
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: --- if (users.empty())
cmb->Append(wxGetUserId());
--- End code ---
ollydbg:
--- Quote from: frithjofh 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
--- End quote ---
Thanks for the contribution! I read the patch, and I see generally its OK, but one question:
What is the difference between
--- Code: ---buffer.GetChar(pos)
--- End code ---
and
--- Code: ---buffer[pos]
--- End code ---
If I remember correctly, in our CodeCompletion's code, it use GetChar, does it have performance issue? Since the operator return a reference?
frithjofh:
@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
frithjofh:
--- Quote ---1, I see you have such change:
--- Code: ---- if (cmb->GetCount()>0)
- cmb->SetSelection(0);
+ cmb->SetSelection(0);
--- End code ---
It is the same thing?
--- End quote ---
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: ---- wxArrayString m_Users;
- wxArrayString m_Types;
std::bitset<(int)tdctError+1> m_supportedTdcts;
DECLARE_EVENT_TABLE()
--- End code ---
When I read the code, I see that inside the function:
--- Code: --- 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());
--- End code ---
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: ---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();
--- End code ---
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: --- if (users.empty())
cmb->Append(wxGetUserId());
--- End code ---
--- End quote ---
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...
Navigation
[0] Message Index
[#] Next page
[*] Previous page
Go to full version