Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."

[stahta01]

I am still working on the header rules that Code::Blocks seems to follow, but a rule it breaks it a few spots.

Here is the main spot it is broken in src/include/configmanager.h including wx/wx.h.

Rule proposed by me.

Do not include wx/wx.h inside the Code::Blocks SDK headers.

Is this a good rule?

[killerbot]

yes, it is not good to include this. It brings to much things into scope.
And certainly never include it in a header file.

just include what you actually need, and use forward declarations if that is sufficient.


Very well spotted !!!

And when it comes to pch, this should be the pattern :
1) outside sdk :

Code

#include "sdk.h"
#ifndef CB_PRECOMP
#include headers from the sdk.h we actually need
#endif
#include all other headers we need who are not in sdk.h

2) inside sdk :

Code

#include "sdk_precomp.h"
#ifndef CB_PRECOMP
#include headers from the sdk.h we actually need
#endif
#include all other headers we need who are not in sdk.h

These headers from/not in  sdk.h include a bunch of Cb headers and a  bunch of wx headers

Both constructs will end up at sdk_common.h, which is today like this :

Code

#ifndef SDK_COMMON_H
#define SDK_COMMON_H

//This file should be included only by sdk.h and sdk_precomp.h
//It includes all the common and necessary header files for precompilation.

/*
 * Enable backwards-compatibility for gcc 3.3 and lower.
 * Although the compiler does not support precompiled headers, the build might still use them.
 * We might significantly reduce the compile time for old compilers, by undefining CB_PRECOMP and thus
 * not including every header file twice.
 * This also allows us to reliably shortcut some includes for compilers that *do* support precompilation.
 */
#if defined(__GNUC__) && !defined(__APPLE__)
    #if ( (__GNUC__ < 3) || ( (__GNUC__ == 3) && (__GNUC_MINOR__ < 4) ) )
        #undef CB_PRECOMP
    #endif
#endif // __GNUC__ && !__APPLE__


#if ( defined(CB_PRECOMP) && !defined(WX_PRECOMP) )
    #define WX_PRECOMP
#endif // CB_PRECOMP

// basic wxWidgets headers : this one itself will check for precompiled headers
// and if so will include a list of wx headers, at the bottom we add some more headers
// in the case of precompilation (note : some headers are in both lists)
// so even if NO CB_PRECOMP we can still have WX_PRECOMP turned on in this "wxprec" header
#include <wx/wxprec.h>

#ifdef __BORLANDC__
	#pragma hdrstop
#endif

#include "prep.h" // this is deliberately not inside the #ifdef block

#ifdef CB_PRECOMP

    // some common wxWidgets headers
    #include <wx/arrstr.h>
    #include <wx/button.h>
    #include <wx/checkbox.h>
    #include <wx/checklst.h>
    #include <wx/choice.h>
    #include <wx/colordlg.h>
    #include <wx/combobox.h>
    #include <wx/confbase.h>
    #include <wx/datetime.h>
    #include <wx/dialog.h>
    #include <wx/dir.h>
    #include <wx/dynarray.h>
    #include <wx/event.h>
    #include <wx/file.h>
    #include <wx/filename.h>
    #include <wx/font.h>
    #include <wx/frame.h>
    #include <wx/fs_zip.h>
    #include <wx/hashmap.h>
    #include <wx/image.h>
    #include <wx/imaglist.h>
    #include <wx/intl.h>
    #include <wx/list.h>
    #include <wx/listbox.h>
    #include <wx/listctrl.h>
    #include <wx/log.h>
    #include <wx/menu.h>
    #include <wx/menuitem.h>
    #include <wx/msgdlg.h>
    #include <wx/notebook.h>
    #include <wx/panel.h>
    #include <wx/print.h>
    #include <wx/process.h>
    #include <wx/radiobox.h>
    #include <wx/radiobut.h>
    #include <wx/regex.h>
    #include <wx/sizer.h>
    #include <wx/socket.h>
    #include <wx/spinctrl.h>
    #include <wx/splitter.h>
    #include <wx/stattext.h>
    #include <wx/string.h>
    #include <wx/textctrl.h>
    #include <wx/thread.h>
    #include <wx/timer.h>
    #include <wx/toolbar.h>
    #include <wx/treectrl.h>
    #include <wx/txtstrm.h>
    #include <wx/utils.h>
    #include <wx/wfstream.h>
    #include <wx/wxscintilla.h>
    #include <wx/xrc/xmlres.h>

    // basic headers
    #include "settings.h"
    #include "globals.h"
    #include "sdk_events.h"
    #include "cbexception.h"

    // absolute base classes
    #include "editorbase.h"
    #include "cbeditor.h"
    #include "compileoptionsbase.h"
    #include "compiletargetbase.h"
    #include "projectbuildtarget.h"
    #include "projectfile.h"
    #include "cbplugin.h"
    #include "cbproject.h"
    #include "cbtool.h"
    #include "cbworkspace.h"
    #include "messagelog.h"
    #include "simpletextlog.h"
    #include "simplelistlog.h"
    #include "compilerfactory.h"
    #include "compiler.h"
    #include "workspaceloader.h"
    #include "editorcolourset.h"
    #include "pipedprocess.h"

    // managers
    #include "manager.h"
    #include "configmanager.h"
    #include "editormanager.h"
    #include "messagemanager.h"
    #include "projectmanager.h"
    #include "menuitemsmanager.h"
    #include "scriptingmanager.h"
    #include "toolsmanager.h"
    #include "templatemanager.h"
    #include "macrosmanager.h"
    #include "pluginmanager.h"
    #include "personalitymanager.h"
    #include "uservarmanager.h"
    #include "filemanager.h"

    // other base files
    #include "xtra_classes.h"
    #include "xtra_res.h"
    #include "safedelete.h"
    #include "infowindow.h"
    #include "licenses.h"

#endif // CB_PRECOMP

#endif // SDK_COMMON_H

Note the above patterns are for cpp files, not for headers, headers should be pch constructs free, and minimal and complete!

Note to myself : this reminds me of my scratchpad article, that I still haven't published : bad me :-(

[stahta01]

See Patch

"Remove include of wx/wx.h from configmanager.h"
https://developer.berlios.de/patch/index.php?func=detailpatch&patch_id=2139&group_id=5358

Only tested on Windows using non precompiled headers.

Tim S

[killerbot]

will take care of the patch tomorrow and in the meantime check on linux ...

[dje]

Hi !

For plugin development, should I use

Code

#include "sdk.h"
#ifndef CB_PRECOMP
#include headers from the sdk.h we actually need
#endif
#include all other headers we need who are not in sdk.h

for classes that have nothing to do with C::B (use of wxString and wxFile for example) or wx.h ?

Dje

[killerbot]

yes, because sdk.h/CB_PRECOMP handles the precompiled headers for CB sdk and wx.
You could do wx independently, but it's better to be do like CB itself.

Or even (better) : no pch  :mrgreen: :mrgreen: :mrgreen:

[killerbot]

@Tim : on a quick look, why all those includes of wx/frame.h ??

EDIT :

The only thing I can think of is the calls to Manager::Get()->GetAppWindow() return a wxFrame*, and are then handed over to methods that (probably) require a wxWindow*.

And since no include of wx/frame.h, nobody can tell wxFrame inherits from wxWindow.

IF this is the reason I see some other/better fixes :

- everywhere cast to wxWindow*  (don't like this one : ugly)
- have Manager::Get()->GetAppWindow() return a wxWindow*, then it is more in synch with the name !!! And I feel for most uses it will be ok.

[dje]

yes, because sdk.h/CB_PRECOMP handles the precompiled headers for CB sdk and wx.
You could do wx independently, but it's better to be do like CB itself.

Ok, just a little preprocessing overhead...

[stahta01]

@Tim : on a quick look, why all those includes of wx/frame.h ??

EDIT :

The only thing I can think of is the calls to Manager::Get()->GetAppWindow() return a wxFrame*, and are then handed over to methods that (probably) require a wxWindow*.

And since no include of wx/frame.h, nobody can tell wxFrame inherits from wxWindow.

IF this is the reason I see some other/better fixes :

- everywhere cast to wxWindow*  (don't like this one : ugly)
- have Manager::Get()->GetAppWindow() return a wxWindow*, then it is more in synch with the name !!! And I feel for most uses it will be ok.

At least 75% of them was because of wxFrame to wxWindow conversion.

Edit: I have no idea about "Manager::Get()->GetAppWindow() return a wxWindow*" , I am still not knowledgeable enough about C:B and C++ to have an informed guess on it. But, I think the "cast to wxWindow*" looks ugly to me, also.

It violates the rules I am thinking of, but we could add an include of wx/frame.h to manager.h which declares GetAppWindow.

Tim S

[stahta01]

It violates the rules I am thinking of, but we could add an include of wx/frame.h to manager.h which declares GetAppWindow.
Tim S

I am going to re-write my patch by adding an include of wx/frame.h to manager.h; this looks like the best solution to me.

Tim S

[killerbot]

wait wait, the include should not be needed in Manager.h --> no knowledge is needed there, the forward declaration is sufficient.
I am gonna try out first the window pointer.

[killerbot]

most can do with the wxWindow*, 5 places need a frame* --> added method GetAppFrame()

[thomas]

I don't understand why this is necessary. wxFrame derives from wxWindow, so if I get a wxWindow*, I can use it as wxFrame*.
Why do we need an extra function to "down"-cast the pointer? Personally, I don't think we should replace things that work correctly with less flexible alternatives if there is no urgent reason (maybe I just don't see the reason?).

The wx.h header inclusion has little impact, since the header guards will already be defined due to precompiled headers, but it is certainly true that we can avoid loading the file in the first place, good idea.

[killerbot]

@Tim : could you update to rev 4381 and adjust your patch to that one, I think it will shrink a lot ;-)

Note : I build on windows (pch) and linux (non pch).
EDIT: linux : not ok for th moment, problem with wxsradiobox.cpp, it seems that wxUSE_RADIOBOX is not defined, so the include of wx/radiobox.h doesn't seem to bring a lot [autotools build !! dunno how to fix that for the moment]

I will then further address your patch (in case some things are still left in there, I guess there will ;-) )

@Thomas : I agree and disagree, GetAppWindow() now returns a wxWindow*, which you can cast to a wxFrame*, but that because actually you know it is a frame, but there's no guarantee,  therefor I made an explicit function, which you can trust ;-)

EDIT2 : adjust 1 more file of wxSmith (and committed) , and on my own system currently I have modified 2 files (wxsradiobox.cpp and wxschecklistbox.cpp : I have them include for *now* wx/wx/.h) but with a solution I don't like. But currebtly i can further test, since rev 4382 broke my entire linux build [../../src/include/logger.h:4:25: error: calling fdopen: Bad file descriptor]

[thomas]

Yes, but before, it returned a wxFrame* so it was safe to assume it is a wxFrame, or you could use it as wxWindow* if you wanted to do that. I just don't see how it is better now 

Don't get me wrong... I mean, it's all nice and good, just it doesn't seem to be much better, but it possibly invalidates quite a bit of code.

From the past, I remember having wxWidgets 2.6/2.8 issues with wxWindow/wxFrame, relating to the window placement code. They moved one member function (forgot which one) from wxFrameto wxWindow between releases, and this was just the one that I had used. So possibly this breaks for 2.6 now again (have not tested). True, we officially support 2.8 now, but that doesn't mean that it *must* break for 2.6 

Ah, writing this, I see you have already edited about 35 depending sources to reflect the changes...  so forget anything I said