wxNewId() in a header file is wrong, right?

[ollydbg]

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.

[thomas]

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.

[ollydbg]

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.

[ollydbg]

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.

[golgepapaz]

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.

[thomas]

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.

[ollydbg]

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. )


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.

[ollydbg]

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