Author Topic: wxNewId() in a header file is wrong, right?  (Read 16106 times)

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
wxNewId() in a header file is wrong, right?
« on: June 03, 2013, 10:09:55 am »
I found such code snippet in plugins\codecompletion\systemheadersthread.h

Code
namespace SystemHeadersThreadHelper
{
    static long idSystemHeadersThreadCompleted = wxNewId();
    static long idSystemHeadersThreadUpdate    = wxNewId();
    static long idSystemHeadersThreadError     = wxNewId();
}

I think it is wrong, because wxNewId() will return different value between each translation unit(cpp file) include (either directly or indirectly) the systemheadersthread.h.

What's your idea? Thanks.


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 thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: wxNewId() in a header file is wrong, right?
« Reply #1 on: June 03, 2013, 12:40:14 pm »
This is probably only included once (otherwise it wouldn't work at all!), so it's kind of OK. But woe if this assumption isn't true one day in the future.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: wxNewId() in a header file is wrong, right?
« Reply #2 on: June 03, 2013, 02:34:13 pm »
Strange:
We have
Code

    Connect(SystemHeadersThreadHelper::idSystemHeadersThreadUpdate,    wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
    Connect(SystemHeadersThreadHelper::idSystemHeadersThreadCompleted, wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadCompletion));
    Connect(SystemHeadersThreadHelper::idSystemHeadersThreadError,     wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadError)     );
}
In CodeCompletion.cpp

and
Code
        // check the dir is ready for traversing
        wxDir dir(dirs[i]);
        if ( !dir.IsOpened() )
        {
            wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, SystemHeadersThreadHelper::idSystemHeadersThreadError);
            evt.SetClientData(this);
            evt.SetString(wxString::Format(_T("SystemHeadersThread: Unable to open: %s"), dirs[i].wx_str()));
            wxPostEvent(m_Parent, evt);
            continue;
        }

in systemheadersthread.cpp.

The header file was included at least in both codecompletion.cpp and systemheadersthread.cpp.

Strange. It looks like it is already a bug. I'm going to test 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 ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: wxNewId() in a header file is wrong, right?
« Reply #3 on: June 03, 2013, 03:31:19 pm »
Strange. It looks like it is already a bug. I'm going to test it.
I set breakpoints in the function body of:
Code
CodeCompletion::OnSystemHeadersThreadUpdate
CodeCompletion::OnSystemHeadersThreadCompletion
CodeCompletion::OnSystemHeadersThreadError
But they never get hit.
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 golgepapaz

  • Multiple posting newcomer
  • *
  • Posts: 44
Re: wxNewId() in a header file is wrong, right?
« Reply #4 on: June 03, 2013, 04:03:33 pm »
It's obvious that the handler is registered to different eventID than the eventID that is
actually send (if it is ever sent of course.).Making them extern and providing the definition in
the cpp file is the obvious solution.

Offline thomas

  • Administrator
  • Lives here!
  • *****
  • Posts: 3979
Re: wxNewId() in a header file is wrong, right?
« Reply #5 on: June 03, 2013, 04:25:39 pm »
But they never get hit.
Well, of course not, how could they :)

If that header is included in more than one source, it's certainly a bug.
"We should forget about small efficiencies, say about 97% of the time: Premature quotation is the root of public humiliation."

Offline ollydbg

  • Developer
  • Lives here!
  • *****
  • Posts: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: wxNewId() in a header file is wrong, right?
« Reply #6 on: June 03, 2013, 05:08:11 pm »
Hi, thomas and golgepapaz, thanks, this is certainly one bug.

I actually find another bug which can also cause the event handler not called. (even I fix the previous bug)
See:
Code
Connect(SystemHeadersThreadHelper::idSystemHeadersThreadUpdate,    wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
Should be:
Code
Connect(idSystemHeadersThreadUpdate,    wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
I'm not sure why compiler not complain this....Oh, the Connect have overload functions:
Code
class WXDLLIMPEXP_BASE wxEvtHandler : public wxObject
{
public:
    wxEvtHandler();
    virtual ~wxEvtHandler();

    wxEvtHandler *GetNextHandler() const { return m_nextHandler; }
    wxEvtHandler *GetPreviousHandler() const { return m_previousHandler; }
    void SetNextHandler(wxEvtHandler *handler) { m_nextHandler = handler; }
    void SetPreviousHandler(wxEvtHandler *handler) { m_previousHandler = handler; }

    void SetEvtHandlerEnabled(bool enabled) { m_enabled = enabled; }
    bool GetEvtHandlerEnabled() const { return m_enabled; }

    // process an event right now
    virtual bool ProcessEvent(wxEvent& event);

    // add an event to be processed later
    void AddPendingEvent(wxEvent& event);

    void ProcessPendingEvents();

#if wxUSE_THREADS
    bool ProcessThreadEvent(wxEvent& event);
#endif

    // Dynamic association of a member function handler with the event handler,
    // winid and event type
    void Connect(int winid,
                 int lastId,
                 int eventType,
                 wxObjectEventFunction func,
                 wxObject *userData = (wxObject *) NULL,
                 wxEvtHandler *eventSink = (wxEvtHandler *) NULL);

    // Convenience function: take just one id
    void Connect(int winid,
                 int eventType,
                 wxObjectEventFunction func,
                 wxObject *userData = (wxObject *) NULL,
                 wxEvtHandler *eventSink = (wxEvtHandler *) NULL)
        { Connect(winid, wxID_ANY, eventType, func, userData, eventSink); }

    // Even more convenient: without id (same as using id of wxID_ANY)
    void Connect(int eventType,
                 wxObjectEventFunction func,
                 wxObject *userData = (wxObject *) NULL,
                 wxEvtHandler *eventSink = (wxEvtHandler *) NULL)
        { Connect(wxID_ANY, wxID_ANY, eventType, func, userData, eventSink); }

The wrong code choose the third Connect function (it just pass winid to eventType. Damn. :o)


Here is the patch to fix all of the two bug:
Code
Index: codecompletion.cpp
===================================================================
--- codecompletion.cpp (revision 9132)
+++ codecompletion.cpp (working copy)
@@ -575,9 +575,9 @@
     Connect(idEditorActivatedTimer, wxEVT_TIMER, wxTimerEventHandler(CodeCompletion::OnEditorActivatedTimer));
     Connect(idAutocompSelectTimer,  wxEVT_TIMER, wxTimerEventHandler(CodeCompletion::OnAutocompSelectTimer) );
 
-    Connect(SystemHeadersThreadHelper::idSystemHeadersThreadUpdate,    wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
-    Connect(SystemHeadersThreadHelper::idSystemHeadersThreadCompleted, wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadCompletion));
-    Connect(SystemHeadersThreadHelper::idSystemHeadersThreadError,     wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadError)     );
+    Connect(idSystemHeadersThreadUpdate,    wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
+    Connect(idSystemHeadersThreadCompleted, wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadCompletion));
+    Connect(idSystemHeadersThreadError,     wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadError)     );
 }
 
 CodeCompletion::~CodeCompletion()
@@ -595,9 +595,9 @@
     Disconnect(idEditorActivatedTimer, wxEVT_TIMER, wxTimerEventHandler(CodeCompletion::OnEditorActivatedTimer));
     Disconnect(idAutocompSelectTimer,  wxEVT_TIMER, wxTimerEventHandler(CodeCompletion::OnAutocompSelectTimer) );
 
-    Disconnect(SystemHeadersThreadHelper::idSystemHeadersThreadUpdate,    wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
-    Disconnect(SystemHeadersThreadHelper::idSystemHeadersThreadCompleted, wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadCompletion));
-    Disconnect(SystemHeadersThreadHelper::idSystemHeadersThreadError,     wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadError)     );
+    Disconnect(idSystemHeadersThreadUpdate,    wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadUpdate)    );
+    Disconnect(idSystemHeadersThreadCompleted, wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadCompletion));
+    Disconnect(idSystemHeadersThreadError,     wxEVT_COMMAND_MENU_SELECTED,wxCommandEventHandler(CodeCompletion::OnSystemHeadersThreadError)     );
 
     while (!m_SystemHeadersThreads.empty())
     {
Index: systemheadersthread.cpp
===================================================================
--- systemheadersthread.cpp (revision 9134)
+++ systemheadersthread.cpp (working copy)
@@ -49,6 +49,13 @@
     #define TRACE2(format, args...)
 #endif
 
+
+
+long idSystemHeadersThreadCompleted = wxNewId();
+long idSystemHeadersThreadUpdate    = wxNewId();
+long idSystemHeadersThreadError     = wxNewId();
+
+
 // internal class declaration of HeaderDirTraverser (implementation below)
 
 class HeaderDirTraverser : public wxDirTraverser
@@ -128,7 +135,7 @@
         wxDir dir(dirs[i]);
         if ( !dir.IsOpened() )
         {
-            wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, SystemHeadersThreadHelper::idSystemHeadersThreadError);
+            wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, idSystemHeadersThreadError);
             evt.SetClientData(this);
             evt.SetString(wxString::Format(_T("SystemHeadersThread: Unable to open: %s"), dirs[i].wx_str()));
             wxPostEvent(m_Parent, evt);
@@ -145,7 +152,7 @@
         if ( TestDestroy() )
             break;
 
-        wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, SystemHeadersThreadHelper::idSystemHeadersThreadUpdate);
+        wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, idSystemHeadersThreadUpdate);
         evt.SetClientData(this);
         evt.SetString(wxString::Format(_T("SystemHeadersThread: %s , %lu"), dirs[i].wx_str(), static_cast<unsigned long>(m_SystemHeadersMap[dirs[i]].size())));
         wxPostEvent(m_Parent, evt);
@@ -153,7 +160,7 @@
 
     if ( !TestDestroy() )
     {
-        wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, SystemHeadersThreadHelper::idSystemHeadersThreadCompleted);
+        wxCommandEvent evt(wxEVT_COMMAND_MENU_SELECTED, idSystemHeadersThreadCompleted);
         evt.SetClientData(this);
         if (!dirs.IsEmpty())
             evt.SetString(wxString::Format(_T("SystemHeadersThread: Total number of paths: %lu"), static_cast<unsigned long>(dirs.GetCount())));
Index: systemheadersthread.h
===================================================================
--- systemheadersthread.h (revision 9132)
+++ systemheadersthread.h (working copy)
@@ -18,12 +18,9 @@
 
 typedef std::map<wxString, StringSet> SystemHeadersMap;
 
-namespace SystemHeadersThreadHelper
-{
-    static long idSystemHeadersThreadCompleted = wxNewId();
-    static long idSystemHeadersThreadUpdate    = wxNewId();
-    static long idSystemHeadersThreadError     = wxNewId();
-}
+extern long idSystemHeadersThreadCompleted;
+extern long idSystemHeadersThreadUpdate;
+extern long idSystemHeadersThreadError;
 
 class SystemHeadersThread : public wxThread
 {


Committed in r9135.
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: 5910
  • OpenCV and Robotics
    • Chinese OpenCV forum moderator
Re: wxNewId() in a header file is wrong, right?
« Reply #7 on: June 04, 2013, 04:47:55 pm »
FYI:
The second bug was introduced in rev8860 around 2013-02-26
Code
* CC: dynamically connect / disconnect events to avoid a RARE race conditions

The first bug was introduced in rev7747 around 2012-02-01
Code
* CC again: major refactoring concerning lockers
* CC: hopefully removed dead-lock
* CC: extracted more classes so they can be used/tested in ParserTest (automake updated)
- updated SVN ignore pattern
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.