Code::Blocks Forums

Developer forums (C::B DEVELOPMENT STRICTLY!) => Development => Topic started by: stahta01 on August 13, 2007, 09:23:15 pm

Title: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 13, 2007, 09:23:15 pm
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.
Quote
Do not include wx/wx.h inside the Code::Blocks SDK headers.

Is this a good rule?
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 13, 2007, 09:31:59 pm
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 :-(
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 13, 2007, 09:49:15 pm
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

Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 13, 2007, 09:56:01 pm
will take care of the patch tomorrow and in the meantime check on linux ...
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: dje on August 13, 2007, 11:02:30 pm
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
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 13, 2007, 11:26:31 pm
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  8) 8) 8) 8) :mrgreen: :mrgreen: :mrgreen:
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 13, 2007, 11:27:21 pm
@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.
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: dje on August 13, 2007, 11:35:06 pm
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...
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 13, 2007, 11:44:33 pm
@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
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 13, 2007, 11:59:24 pm
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
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 14, 2007, 09:40:52 am
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.
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 14, 2007, 10:52:14 am
most can do with the wxWindow*, 5 places need a frame* --> added method GetAppFrame()
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: thomas on August 14, 2007, 11:57:22 am
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.
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 14, 2007, 12:05:55 pm
@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]
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: thomas on August 14, 2007, 12:49:13 pm
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  8)

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  :D

Ah, writing this, I see you have already edited about 35 depending sources to reflect the changes...  so forget anything I said 8)
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 14, 2007, 01:34:49 pm
but not all of those 35 are for the wxFrame/wxWindow issue, those were 10 or so.
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 14, 2007, 04:37:59 pm
@Tim : could you update to rev 4381 and adjust your patch to that one, I think it will shrink a lot ;-)

I am testing my update to SVN 4385, right now.

Tim S
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 14, 2007, 11:49:40 pm
on my linux I got things building, but by changing 2 source files (which I did NOT commit (yet)).

1) wxsradiobox.cpp
2) wxschecklistbox.cpp

Both complain about the fact they don't know their wx class things are dealing with. That's wxRadioBox and wxCheckListBox.
Although we include for those :
#include <wx/radiobox.h>
 and
#include <wx/checklst.h>

When you have a look at these  header they could be empty in case :
wxUSE_RADIOBOX
 and
wxUSE_CHECKLISTBOX
are not defined. Could be that this is the case. However when both cpp files get at first an extra include statement of 'wx/wx.h' then it compiles.

In case those preprocessor things are not defined, how should they be defined, including wx.h doesn't seem to me the correct way.
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 14, 2007, 11:52:52 pm
The normal way is to include wx/defs.h to define wxUSE defines.

Tim S
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 15, 2007, 12:00:27 am
will try this, at first glance (I already looked), wx/defs.h : no sign of for example 'wxUSE_RADIOBOX'
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: killerbot on August 15, 2007, 12:01:52 am
works : will commit
Title: Re: Proposed rule "Do not include wx/wx.h inside the Code::Blocks SDK headers."
Post by: stahta01 on August 15, 2007, 12:19:01 am
will try this, at first glance (I already looked), wx/defs.h : no sign of for example 'wxUSE_RADIOBOX'

Somewhere deep in the includes it includes setup.h and chkconf.h which defines 95% of wxUSE defines.

Tim S