Solved

Attn: RONSLOW, mikeblas, chensu, other MFC gurus

Posted on 2000-02-24
65
634 Views
Last Modified: 2013-11-25
This discussion goes back to a question about how to add items to a CComboBox.  The crux of this is what is the acceptable method to get the pointer to a CComboBox (or other) MFC class in a window.

RONSLOW says:

"Don't use GetDlgItem as it only returns a CWnd (ie not the appropriate class), so calling member functions may stuff up."

I don't know what "stuff up" means exactly but I gather it's a bad thing.

I contend that the following is a perfectly acceptable, legal C++/MFC, and safe way of getting the pointer to the CComboBox control in the dialog:

CComboBox *c = (CComboBox *)GetDlgItem(IDC_COMBO);

I contend that this is legal for ANY CWnd derived class in an MFC application. Not only have I always used this method and have NEVER once had a failure with it (except when using an invalid ID), I'll quote several references to support this:

1) MFC Docs.  See CWnd::GetDlgItem where it gives the example:

CEdit* pBoxOne;
pBoxOne = (CEdit*) GetDlgItem(IDC_EDIT1);

This code (or similar) is also littered throughout the MFC samples.  There are too many to reference here.

2) Marshall Brain & Lance Lovett, "Visual C++ 2", Prentice Hall 1995, pg. 142:

"...GetDlgItem function is useful for converting the control's ID in the dialog into an actual CEdit pointer so you can call CEdit member functions"

3) Mike Blaszczak, "Professional MFC with Visual C++ 5", Wrox Press, 1997, pg. 223:

"...MFC implements a GetDlgItem() function of its own, as a member of the CWnd class.  The MFC version of the function returns a pointer to a CWnd instead of a handle to a window.  You can then CAST THIS CWnd POINTER TO A POINTER AT ANY CONTROL CLASS you'd like."  (emphasis mine)

I could dig up more the this should be sufficient to backup my point that this is not only Microsoft recommended practice but a technique documented in books by leading MFC authors.

RONSLOW also takes exception to my bringing COM into this.  I contend that COM works with what he calls "blindly casted" pointers for the same reason that MFC can do it.  Namely that C++ maintains the vtbl order of functions for dervied classes.  RONSLOW says:

"Your comparison to COM is like comparing apples to oragnges.

COM cast from a pointer-to-base-class (that is POINTING TO an object of the DERIVED class) to a pointer-to-derived.  Quite legitimate as long as your "know" that the object being pointer to is of the correct type."

Like I said, COM works fine for the same reason that MFC CWnd derived classes work with a cast.  The vtbl is compatible and you can cast a CWnd * to a CEdit *, CButton *, CComboBox *, etc...  In COM all pointers are derived from IUnknown.  Therefore you can cast an IUnknown * to an IDispatch *, etc.

Now I realize that there is more to COM that this, but it's an important point.  For a given C++ compiler you can count on the structure of the member function order in the class binary object.  It won't change unexpectedly and cause your application to "stuff up".

I've been doing it this way for a long time, but if I'm wrong (and others are as well) I'd like to understand why and also understand why my applications are NOT failing in the field due to this.

Again, RONSLOW says:

"Please .. listen and learn .. for your own sake and that of those you help here and elsewhere.  It is BAD CODING.  PERIOD."

Please control your urge to post an answer.  Comments only. I'll award the
points to whoever offers the best explanation of why RONSLOW is right (and Brain, mikeblas, and Microsoft are wrong!) or why I'm OK on this and RONSLOW is overreacting.

Further, if the balance of opinion is that RONSLOW is correct, I'll also post another 200 pt question for him as a reward for being right.

0
Comment
Question by:jhance
  • 40
  • 11
  • 5
  • +5
65 Comments
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556363
Here is what Paul DiLascia
says...  Want to take it up with him???

Q. In the September 1997 issue you wrote that code like this is technically incorrect:

CMyDialog::SomeFn( ... )
{  
    CEdit* pEdit = (CEdit*)GetDlgItem(ID_EDIT1); // NOT!  
    pEdit->GetSel( ... );
}

But we’ve seen this type of code used in many books as a way to access a control. Is there a procedure that would make the above sample technically correct—that is, a procedure other than using DDX/DDV or subclassing the control (which I believe would create a permanent map entry)?

Doug Kehn and Ray Marshall

A. There is another way, but not one that avoids creating a permanent map entry. First, let me quickly remind those of you who didn’t read the September issue why the cast is incorrect. It’s because GetDlgItem returns a CWnd, not a CEdit. You can easily see this by using MFC runtime classes or C++ dynamic casting. I wrote a little program called EditCast (see Figure 1) that’s essentially a dialog with a button in it. When you press the button, EditCast executes the following code:

CWnd*  pWnd  = GetDlgItem(IDC_EDIT1);
CEdit* pEdit = dynamic_cast<CEdit*>(pWnd);
BOOL   bIsEdit = pWnd->IsKindOf(RUNTIME_CLASS(CEdit));

Then it prints the values of pWnd, pEdit, and bIsEdit in TRACE diagnostics. Figure 2 shows the output in my TraceWin utility. As you can see, doing a C++ dynamic cast to CEdit fails (the cast returns NULL); likewise, CObject::IsKindOf returns FALSE, indicating that the pointer does not point to a CEdit-derived object. And yet, following the above lines of code, EditCast next executes these lines, which work perfectly fine:

pEdit = (CEdit*)pWnd;
pEdit->SetSel(0, -1);
pEdit->ReplaceSel("");
pEdit->SetFocus();

When you press the button, the code deletes whatever was in the edit control. So what gives?

As I explained in September, the code works because the CEdit functions are merely thin wrappers that send messages to the window. For example, ReplaceSel sends an EM_REPLACESEL to the control:

// in afxwin2.inl
inline void
CEdit::ReplaceSel(LPCTSTR lpsz, BOOL bCanUndo)
{
    ::SendMessage(m_hWnd, EM_REPLACESEL,
                  (WPARAM)bCanUndo, (LPARAM)lpsz);
}

Since the control really is an edit control, it responds to EM_
REPLACESEL by doing what you expect. The only reason it works is that CEdit contains no data members or virtual functions. If ReplaceSel had been a virtual function instead of inline, calling it would call the CWnd function, not the CEdit one, no matter how you cast. C++ would call through the function pointer in the vtable, which would be CWnd::ReplaceSel. And if ReplaceSel used some data member that was part of CEdit and not CWnd, the code would crash or do something unpredictable because the CWnd object would not have this data.

Figure 1: EditCast.cpp

////////////////////////////////////////////////////////////////
// EditCast 1997 Microsoft Systems Journal.
// If this program works, it was written by Paul DiLascia.
// If not, I don't know who wrote it.
// EditCast illustrates why it’s not safe to cast the return from
// CWnd::GetDlgItem to a CEdit or other kind of window class.

#include <afxwin.h>         // MFC core and standard components
#include <afxext.h>         // MFC extensions
#include "resource.h"
#include "TraceWin.h"

#ifdef _DEBUG
#define new DEBUG_NEW
#undef THIS_FILE
static char THIS_FILE[] = __FILE__;
#endif
//////////////////
// Dialog class that alters the TAB sequence and handles
// the RETURN key.
//
class CMyDialog : public CDialog {
public:
    CMyDialog();
    ~CMyDialog();
    virtual BOOL OnInitDialog();
    afx_msg void OnButton();
    DECLARE_MESSAGE_MAP()
};
////////////////////////////////////////////////////////////////
// Application class
//
class CApp : public CWinApp {
public:
    CApp() { }
    virtual BOOL InitInstance();
} theApp;

/////////////////
// Initialize: just run the dialog and quit.
//
BOOL CApp::InitInstance()
{
    CMyDialog dlg;
    dlg.DoModal();
    return FALSE;
}
////////////////////////////////////////////////////////////////
// CMyDialog
//
BEGIN_MESSAGE_MAP(CMyDialog, CDialog)
    ON_BN_CLICKED(IDC_BUTTON1, OnButton)
END_MESSAGE_MAP()

//////////////////
// Construct dialog: set everything to zero or NULL.
//
CMyDialog::CMyDialog() : CDialog(IDD_DIALOG1)
{
}

CMyDialog::~CMyDialog()
{
}
/////////////////
// Initialize dialog: load accelerators and set initial focus.
//
BOOL CMyDialog::OnInitDialog()
{
    return CDialog::OnInitDialog();
}
/////////////////
// Button handler: do demo stuff
//
void CMyDialog::OnButton()
{
    // Get the window
    CWnd*     pWnd  = GetDlgItem(IDC_EDIT1);
    ASSERT(pWnd);
    // Demonstrate that pWnd is not an edit control.
    CEdit*    pEdit = dynamic_cast<CEdit*>(pWnd);
    BOOL      bIsEdit = pWnd->IsKindOf(RUNTIME_CLASS(CEdit));
    TRACE("GetDlgItem(IDC_EDIT1) returns CWnd = %p\n", pWnd);
    TRACE("dynamic_cast to CEdit* = %p\n", pEdit);
    TRACE("pWnd->IsKindOf(RUNTIME_CLASS(CEdit)) returns %d\n", bIsEdit);
   
    // But you can treat it as one! (Do this at your own risk)
    pEdit = (CEdit*)pWnd;
    pEdit->SetSel(0, -1);
    pEdit->ReplaceSel("");
    pEdit->SetFocus();
}



Figure 2: TraceWin Utility

So what’s the correct way to avoid the cast? As you point out, the normal thing to do is put a CEdit object in your dialog and then subclass it:

class CMyDialog : public CDialog {
    CEdit m_edit;
    virtual BOOL OnInitDialog() {
        m_edit.SubclassDlgItem(IDC_EDIT, this);
        return CDialog::OnInitDialog();
    }
·

·

·

};

Now if you call GetDlgItem, you get a CEdit object since m_edit is in the permanent map. Remember, CWnd::GetDlgItem only creates a CWnd on-the-fly if it can’t find one in the permanent map. Using a dialog object is the preferred way to access controls, but there are times when this may not be feasible. Say you’re writing some library code and didn’t create the edit control, but had it passed to you. In that case, you can access the edit control like so:

CMyDialog::SomeFn( ... )
{
    CEdit edit;
        edit.Attach(::GetDlgItem(m_hWnd, IDC_EDIT));
        edit.SetSel( ... );
·

·

·

        edit.Detach();
}

In this example, ::GetDlgItem (the Windows API function, not the CWnd function) returns the HWND of the control and I attach it to a local CEdit object. CWnd::Attach adds the CEdit object to the permanent map and sets CEdit.m_hWnd to the HWND of the edit control. Just make sure you don’t forget to Detach the control when you’re done—otherwise the CWnd destructor will destroy the actual control. Of course, this trick will fail if the control is already attached to a CWnd-derived object.

If you want to be absolutely safe, you could write something like this:

HWND hwnd = GetDlgItem(IDC_EDIT);
CEdit* pEdit =  
     CWnd::FromHandlePermanent(hwnd);
BOOL bMine;
if (pEdit) {
    ASSERT_KINDOF(CEdit, pEdit);
} else {
    pEdit = new CEdit;
    pEdit->Attach(hwnd);
    bMine = TRUE;
}
·

·

·

if (bMine) {
    pEdit->Detach();
    delete pEdit;
}

CWnd::FromHandlePermanent only returns a CWnd pointer if the window has a permanent object attached to it. If not, I create a CEdit and attach it. In general, this technique can be used whenever you’re given an HWND from somewhere that you know is an edit control (or static, or button) and you want to treat it as such.

I often use the local variable technique to program GDI. For example, there are many messages and callbacks where Windows® gives you a device context in the form of an HDC. To access it with MFC, you can write:

 CDC dc;
 dc.Attach(hdc);
 dc.SelectObject(...);
 ·

 ·

 ·

 dc.Detach();

This is the equivalent of writing:

CDC& dc = *CDC::FromHandle(hdc);

The difference is that you save a memory allocation since the CDC object is a stack variable. On the other hand, if you use FromHandle you don’t have to worry about detaching since MFC takes care of detaching and destroying the temporary CDC object during its idle processing. In general, it’s probably safer to use FromHandle since it returns the permanent object if there is one. This applies to all MFC objects—windows, device contexts, brushes, and so on.

0
 
LVL 23

Expert Comment

by:chensu
ID: 2556378
Nothing much to say since RONSLOW's comment is pretty complete.

To recap,

"the code works because the CEdit functions are merely thin wrappers that send messages to the window."

"The only reason it works is that CEdit contains no data members or virtual functions. If ReplaceSel had been a virtual function instead of inline, calling it would call the CWnd function, not the CEdit one, no matter how you cast. C++ would call through the function pointer in the vtable, which would be CWnd::ReplaceSel. And if ReplaceSel used some data member that was part of CEdit and not CWnd, the code would crash or do something unpredictable because the CWnd object would not have this data."


And I would prefer to use the DDX mechanism, which actually does subclass for you.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556382
>1) MFC Docs.  See CWnd::GetDlgItem where it gives the example:
>
>CEdit* pBoxOne;
>pBoxOne = (CEdit*) GetDlgItem(IDC_EDIT1);
>
>This code (or similar) is also littered throughout the MFC samples.
>There are too many to reference here.

There aren't that many that do the case.  And it is still wrong (MFC does aren't perfect .. look at the number of corrections in the KB)

>2) Marshall Brain & Lance Lovett, "Visual C++ 2", Prentice Hall 1995, pg. 142:
>
>"...GetDlgItem function is useful for converting the control's ID in the
>dialog into an actual CEdit pointer so you can call CEdit member functions"

They are wrong (in general), it does NOT point to a CEdit object, so you cannot legitimately call CEdit members

>3) Mike Blaszczak, "Professional MFC with Visual C++ 5", Wrox Press, 1997, pg. 223:
>
>"...MFC implements a GetDlgItem() function of its own, as a member of the CWnd class.
>The MFC version of the function returns a pointer to a CWnd instead of a handle to a window.
>You can then CAST THIS CWnd POINTER TO A POINTER AT ANY CONTROL CLASS you'd like."  (emphasis mine)

Mike is wrong here too.

And, to explain again why

consider

class A {};
class B : public A {
public:
  int m_data;
};
A a;
B* pB;
pB = (B*)(&a);
pB->m_data = 1; // access violation!!

This is the sort of thing that (can) happen with this bad casting of the return value of GetDlgItem.

I even have a very simple dialog-app that demonstrates it happening.
0
 
LVL 32

Author Comment

by:jhance
ID: 2556386
Well, there certainly seems to be a difference of opinion, Brain, Blaszczak, and Microsoft say it's OK, DiLascia says it's not.

My opinion is that this statement that he makes is the key:

"The only reason it works is that CEdit contains no data members or virtual functions"

Guess what, CEdit, et. al. don't have data members or virtual functions.  So they always work as expected.  

I submit (again) that it's not wrong, therefore, to do it this way.  

DiLascia's point is more applicable to casting in the general case and arguments have been made (mostly by purists who program for the sake of programming) that casts don't even belong in a language like C++.  

It works, not by accident, but rather by design.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556388
Chensu seems to agree that my comment quoting Paul is proof enough.  Maybe I should go for the 200 points proof myself?

0
 
LVL 32

Author Comment

by:jhance
ID: 2556390
>And, to explain again why

>consider

>class A {};


NO!!!  Again, you're falling back to a general purpose case.  This is a specific question about MFC control classes!  The original question was about CComboBox, not "class A".

I'm not buying this argument.  It's not applicable.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556395
Here is an example that dies on program exit because of the incorrect use of GetDlgItem..

Create an AppWizard dialog app
Add a list box and combo box to it
Add these lines to the OnInitDialog

// TODO: Add extra initialization here
CCheckListBox* pCheckListBox = (CCheckListBox*)GetDlgItem(IDC_LIST1);
CComboBox* pCombo = (CComboBox*)GetDlgItem(IDC_COMBO1);
pCheckListBox->SetCheckStyle(BS_CHECKBOX);

The last line above changes the m_nStyle memeber of what we have told C++ is a CCheckListBox object (but really is a CTempWnd).  Doing so corrupts the temorary windows map etc and the app crashes.

Proof positive that this sort of access results in things "stuffing up".

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556410
Jhance .. no it works by ACCIDENT.

It just so happens that most of the CWnd derived control classes don't have any virtual functions that you would usually call (but do have _some_) and don't have their own member data.

But NOT ALL of then .. MFC's CCheckListBox has two member vars for example, and a user defined class derived from CComboBox etc may well not be.

You claim that the method is safe for "ANY CWnd derived class "

It isn't.

You claim it is "perfectly acceptable, legal C++".

It isn't.

I'm right. You're wrong. Nyha nyah :-)

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556414
Got you on the back foot now.

So, you're admitting that it is not generally acceptable and legal C++ code.

But it just happens to work for CComboBox and some of the other MFC control classes.

Looks like you're close to admitting you're wrong :-)
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556422
BTW:

jhance said:
>DiLascia's point is more applicable to casting in the general case and arguments have been made
>(mostly by purists who program for the sake of programming) that casts don't even belong in a language like C++.  

No .. his comments are specifically about casting the return value of GetDlgItem to some other CWnd-derived class.

BTW: C++ (dynamic_cast) and MFC (STATIC_DOWNCAST) are safe alternatives to C-style casts (that are more acceptable to the purists).  They won't let you do the cast from GetDlgItem, because the cast isn't valid.
0
 
LVL 11

Accepted Solution

by:
mikeblas earned 200 total points
ID: 2556511
RONSLOW> Mike is wrong here too.

Pedantically, sure. But that's the way people use MFC. And it works.

 jhance> Guess what, CEdit, et. al. don't have data members or virtual functions.  

Actually, they do; with a few exceptions, they're all inhereted from CWnd. There are a few control classes which can't be "bad polymorphed". CRichEditCtrl, for instance, and CCheckedListBox.

 jhance> It works, not by accident, but rather by design.

Exactly.

What's going on here is that MFC is trying to keep the Windows programming model alive. That was a design decision for MFC back in its infancy: it should be pretty obvious why Microsoft wanted to do that.

There's no convenient way for MFC programmers to cast from a generic CWnd* to any CWnd-derived* with complete type safety. You could probably cook one up, but it would be a large perforamnce penalty, and would rely on compiler features that didn't exist back in the days that MFC was written.

For example, DiLasia says "As you point out, the normal thing to do is put a CEdit object in your dialog and then subclass it"  I don't think that's normal at all. And it's far less efficient for infrequently used controls.

Fortunately, MFC doesn't endeavor to take all the sharp edges of off C++.

So, is it "wrong" from a pedantic sense of type safety? Absolutely not. Is it the way I would recommend writing MFC programs? Sure.

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556594
Mike.

>RONSLOW> Mike is wrong here too.
>Pedantically, sure.

No, it is NOT just pedantic.  It can cause real problems .. this isn't just a matter of bad 'style' or being fussy about the language.

One cannot, in general, cast the result of GetDlgItem to any CWnd-derived control class with safety.  That sort of thing is strictly BAD C++ CODING and no amount of excuses about special cases with _some_ MFC classes can prove otherwise.

>But that's the way people use MFC. And it works.

Only sometimes.  See my example earlier with CCheckListBox.  This is an MFC supplied CWnd derivative, and it causes an application to crash when used as suggested.  Doesn't sound like it works to me.

>jhance> Guess what, CEdit, et. al. don't have data members or virtual functions.  
>Actually, they do; with a few exceptions, they're all inhereted from CWnd.
>There are a few control classes which can't be "bad polymorphed".
>CRichEditCtrl, for instance, and CCheckedListBox.

Oh .. so you are admitting that there ARE classes where it doesn't work.  But you still advocate a blanket recommendation that this technique can be used with "any control class you'd like"? (to quote jahnces quote from your book)

>jhance> It works, not by accident, but rather by design.
>Exactly.

It is an ACCIDENT of design that classes like CEdit didn't require any virtuals or members.  But when we come to, say, CCheckListBox when these are required, the "accident" of design no longer applies and the cast causes problems.

>What's going on here is that MFC is trying to keep the Windows programming model alive.
>That was a design decision for MFC back in its infancy:
>it should be pretty obvious why Microsoft wanted to do that.

Couldn't agree more.  BUT we're working with C++ and classes here.  There is no equivalent to what we are doing in the "Windows programming model".  As soon as you start casting etc you're going beyond where that model applies and beyond correct and safe C++ coding.

>There's no convenient way for MFC programmers to cast from a generic
>CWnd* to any CWnd-derived* with complete type safety.

There you go .. you've admitted that I am right.  Thanks.

>You could probably cook one up, but it would be a large perforamnce penalty,
>and would rely on compiler features that didn't exist back in the days that
>MFC was written.

MFC have already 'cooked one up' .. its called subclassing.  And if you don't want to use it for whatever reason, try something like the example at the end of this message (based on Paul's article) (yes .. it uses templates, but they are available now, so lets take advantage of them).

>For example, DiLasia says "As you point out, the normal thing to do is
>put a CEdit object in your dialog and then subclass it"  I don't think that's
>normal at all. And it's far less efficient for infrequently used
>controls.

The subclassing happens once when the dialog is created and doesn't take that long .. from then on you don't need to GetDlgItem anymore so it is more efficient.

And why is it not normal to do this.  Because authors (such as yourself) keep advocating the dangerous and lazy way of doing things.  You really should know better (well, at least now you should :-)

>Fortunately, MFC doesn't endeavor to take all the sharp edges of off C++.

Not sure what that means, so I guess I'll just agree with it.

>So, is it "wrong" from a pedantic sense of type safety? Absolutely not.

Thought you just agreed above that is IS wrong pedantically.  A little consistency please!

>Is it the way I would recommend writing MFC programs? Sure.

True, you may recommend it, but that doesn't make it right OR safe.


example class (see above)
template <class T> class CGetDlgItem {
public:
    CGetDlgItem(int id, CWnd* pWnd)
        : m_bCreatedByMe(false)
        , m_pT(NULL)
    {
        HWND hwnd =NULL;
        pWnd->GetDlgItem(id,&hwnd);
        m_pT = CWnd::FromHandlePermanent(hwnd);
        if (m_pT) {
            ASSERT_KINDOF(T, m_pT);
            m_bCreatedByMe = false;
        } else {
            m_pT = new T;
            m_pT->Attach(hwnd);
            m_bCreatedByMe = true;
        }
    }
    ~CGetDlgItem() {
        if (m_bCreatedByMe) {
            m_pT->Detach();
            delete m_pT;
        }
    }
    operator T* () {
        return m_pT;
    }
    T* operator-> () {
        return m_pT;
    }
private:
    bool m_bCreatedByMe;
    T* m_pT;
};

so in your code instead of

CCheckListBox* pCheckListBox = (CCheckListBox*)GetDlgItem(id);
pCheckListBox->xxxx()

you'd say:

CGetDlgItem<CCheckListBox> pCheckListBox(id,this);
pCheckListBox->xxxx();

looks pretty easy to me !!

0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2556677

 mikeblas> So, is it "wrong" from a pedantic sense of type safety? Absolutely not.

Sheesh!  I meant "Aboslutely", not "absolutely not".

 RONSLOW> See my example earlier with CCheckListBox.  

Which you contrived to prove the point. Sure. If you go out of your way far enough, you can break anything.

 RONSLOW> "any control class you'd like"?

Yep--that should be any control class, as long as it matches what's in the map. Most often, that's going to mean that the class matches the type of window in question. That's an easy thing for programmers to get right.

 RONSLOW> It is an ACCIDENT of design that classes like CEdit didn't require any virtuals or members

It is?  I'm sorry--I don't remember seeing you at any of the team meetings.

 RONSLOW> The subclassing happens once when the dialog is created and doesn't take that long

But it lives forever; an object enters the permanent map. MFC has to step into the message dispatch for that object. Since we're talking about dialog objects, that's normally very many objects.

 RONSLOW> looks pretty easy to me !!

Looks totally unnecessary, to me.

I'm all for going out of the way to earn type safety when it's an easy to have a mismatch that's fatal. But I think there's a comrpomise to be made. Here, in the aging design of MFC, you'll find that you have to go pretty far out of your way (eg, your example) to make any real trouble.

I think your code is also only a partial solution--you can cause trouble with that template quickly, too.

   CGetDlgItem<CRichEditCtrl> pCheckListBox(ID_MYLISTBOX, this);
   pCheckListBox->xxxx();

is just as likely to cause a crash, for instance.

 RONSLOW> A little consistency please!

It's a typo there, tough guy!

By the way, what's in your bonnet? Did someone offer to buy you a new car if you found a flaw in MFC, or something?

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556711
So, your argument seems to be: if you only use it in cases where it works, then it works, otherwise .. well .. I'm not sure what you suggest.

>RONSLOW> See my example earlier with CCheckListBox.  
>Which you contrived to prove the point.
>Sure. If you go out of your way far enough, you can break anything.

It is not at all far out of the way.. The only different is that it is a list box with check marks and not a combo.  Of course it is 'contrived', i don't use bad code like that myself :-).

It is an example I quickly and easily put together to show that the casting method is flawed.  It could EASILY happen in anyone else's application.

>It is?  I'm sorry--I don't remember seeing you at any of the team meetings.

I was the ugly little guy in the corner that noone talked to :-)  Point taken.

So, did they actually say "lets not add any virtual methods or member data to CWnd-derived control classes so that we can safely use bad C++ code"?

And why did they change their mind with CChecklistBox etc and not bother telling anyone that the old methods would now cause a crash?

>   CGetDlgItem<CRichEditCtrl> pCheckListBox(ID_MYLISTBOX, this);
>   pCheckListBox->xxxx();

That is a typo.  The GetDlgItem version is 'correctly typed' code that crashes.  A subtle but important difference.

If one REALLY wanted to be careful, one could check the window class of the control, and encapsulate that as well .. but we don't want or need to go overboard here.

I think it is sufficient (and necessary)to ensure that, when you don't make typos, that the program doesn't crash.  The GetDlgItem cast fails on that case.

>RONSLOW> A little consistency please!
>It's a typo there, tough guy!

Fair enough.  I'm tough enough to say I was wr .. wro .. wron .. wrong (there .. did it)

>By the way, what's in your bonnet?

Nothing (except my brain) .. I just didn't like being told my correct statement that that sort of cast can be dangerous is 'baloney' (as in the original question that spawned this debate).

>Did someone offer to buy you a new car if you found a flaw in MFC, or something?

I'd have a car yard full of them by now if they did.  You offering?
0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2556727
> as in the original question that spawned this debate).

That wasn't my question. I think you should save your venom for someone else.

 > It is not at all far out of the way..

Sure ya're. You're getting the wrong item. (static casts) are to be used when you're sure you've got the right type. You've got the wrong type.

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556753
jhance:

Looks like I've won the debate because you said:

>I contend that this is legal for ANY CWnd derived class in an MFC application.

And that is clearly wrong.  I have shown a very simple example where it doesn't - just try with CCheckListBox as I did.  And

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2556791
>> as in the original question that spawned this debate).
>That wasn't my question.

I know that .. I was there.  But you're here now too, and so I wouldn't want you to feel as though you've missed anything.

>I think you should save your venom for someone else.

Don't worry .. I've plenty to go around .. you won't miss out :-)

>>It is not at all far out of the way..
>Sure ya're. You're getting the wrong item.

No I'm not.

I was doing everything 'right'.  that was the whole point of the example, showing how doing it 'right' crashes.

There was a list box on the dialog, and I got a pointer to it, cast it to be a CCheckListBox (which has a list box as its window) and SPLAT! CORRUPT! APOCALYPSE NOW! on exit.

>(static casts) are to be used when you're sure you've got the right type.
>You've got the wrong type.

My point exactly .. the actual type pointed to is CWnd.  And that is the WRONG type no matter WHAT you try to cast it to (CEdit, CComboBox, CCheckListBox whatever).

Thanks for backing me up .. even if you didn't mean to.

0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2556823
> I was doing everything 'right'.  

If you did it right, it wouldn't crash.

Looks like you forgot to finish your sentence on your penultimate posting, by the way.

..B ekiM
0
 
LVL 23

Expert Comment

by:chensu
ID: 2556836
Most interesting question at EE!


RONSLOW> So, you're admitting that it is not generally acceptable and legal C++ code.

After all, MFC is C++.


jhance> It works, not by accident, but rather by design.

Probably, it works accidentally by design.


My opinion:

Use this technique with extreme caution only when you know what you are doing. Use DDX/subclass whenever possible.
0
 
LVL 32

Author Comment

by:jhance
ID: 2557630
RONSLOW:

// TODO: Add extra initialization here
CCheckListBox* pCheckListBox = (CCheckListBox*)GetDlgItem(IDC_LIST1);
CComboBox* pCombo = (CComboBox*)GetDlgItem(IDC_COMBO1);
pCheckListBox->SetCheckStyle(BS_CHECKBOX);

The last line above changes the m_nStyle memeber of what we have told C++ is a CCheckListBox object (but really is a CTempWnd).  Doing so corrupts the temorary windows map etc and the app crashes.

You must be doing something wrong.  I tried this and it works fine.  Maybe you need to cook up a phony example that crashes better.

Back to my point, however.  I still hold that CWnd::GetDlgItem() works the way it was intended to work.

I've posted references from 3 sources that support my position where you've posted only 1 from an author who (in my opinion) doesn't like MFC since it tends to "bend the rules" of C++ a bit to get it's job done.

I'll award you the points, however, under one condition.  You solemnly swear that you will never write another program using MFC again.  Why do I ask this?  Well, the MFC library _ITSELF_ uses this very dangerous and foolhardy technique.  If you use MFC, _YOU_ are endorsing its method by default.

Don't believe me?  See for youself:

DOCKSTAT.CPP:

CDockBar* pDockBar = (CDockBar*)pDockFrame->GetDlgItem(AFX_IDW_DOCKBAR_FLOAT);

DOCMGR.CPP:

CListBox* pListBox = (CListBox*)GetDlgItem(AFX_IDC_LISTBOX);

PPGCOLOR.CPP:

SetButton((CColorButton *)(GetDlgItem(CColorButton::idClicked)));

PPGFONT.CPP:

GetDlgItem(AFX_IDC_STRIKEOUT)->EnableWindow(FALSE);

GetDlgItem(AFX_IDC_UNDERLINE)->EnableWindow(FALSE);

PPGPICT.CPP:

(Oh No!  It's our old friend the CComboBox!!!!!)

CComboBox* pPropName = (CComboBox*)GetDlgItem(AFX_IDC_PROPNAME);

VIEWCORE.CPP:

return (CScrollBar*)pParent->GetDlgItem(nIDScroll);

VIEWPREV.CPP:

CPreviewView* pView = (CPreviewView*) pFrameWnd->GetDlgItem(AFX_IDW_PANE_FIRST);

WINFRM2.CPP:

CDockBar* pDockBar = (CDockBar*)pDockFrame->GetDlgItem(AFX_IDW_DOCKBAR_FLOAT);


Better luck next time.....
0
 
LVL 32

Author Comment

by:jhance
ID: 2557752
Oh NO!  Another author weighs in on this:

"Is a class derived has a public base class base, then a pointer to derived can be assigned to a variable of type pointer to base without use of explicit type conversion.  The opposite conversion, for pointer to base to pointer to derived, must be explicit. For example:

class base {....};
class derived : public base {...};

derived m;
base *pb = &m;    // implicit conversion
derived *pd = pb; // error: a base * is not a derived *
pd = (derived *)pb; // explicit conversion

Hmmmm that last line looks a lot like:

CComboBox *c = (CComboBox *)GetDlgItem(...);

I'll leave it up to you to discern where this reference comes from.  But I will say that he wrote the book on C++ and, in my opinion at least, outweighs anything DiLascia has to say on this topic.

To be fair, however, this author does indeed warn that this can be misused but that doesn't make it's use wrong.  If you follow the argument that language features that can be misused should be removed from the language, you'll end up with a horrible mess like Basic, Pascal or it's modern soulmate, Java.
0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2559072

 chensu> Use this technique with extreme caution only when you know what you
 chensu> are doing. Use DDX/subclass whenever possible.

That's my opinion, too, I guess. But I also think that it's very rare when you don't have enough certainty to use the cast operators.

   CEdit* pEdit = (CEdit*) GetDlgItem(IDC_EDIT1);

is pretty certainly going to return a good CEdit*. And if it doesn't, you'll find out in short order.

..B ekiM

0
 
LVL 32

Author Comment

by:jhance
ID: 2559144
RONSLOW,

Unless you have something that going to break some new ground on this, I'm leaning toward accepting mikeblas' comment here.  There's been a lot a good discussion and a fair bit of taunting on your part but I find Mike's arguments in favor of this being a valid technique persuasive.

Just to recap...

Evidence in my favor:

MS MFC docs, Brain, Blaszczak, MFC source code, numerous MFC SAMPLE apps all show this being used.  Plus a reference from some guy from AT&T labs about this being a valid C++ technique as well.

Evidence in your favor:

DiLascia's magazine column and an example using CCheckListBox that doesn't crash.

If we were in court, you'd lose because you haven't proved your point by a preponderance of the evidence.

Even your one-time supporter chensu seems to have backpedaled:

"My opinion:

Use this technique with extreme
caution only when you know what you
are doing. Use DDX/subclass whenever
possible."
0
 
LVL 23

Expert Comment

by:chensu
ID: 2559753
jhance> Even your one-time supporter chensu seems to have backpedaled

I have not changed my mind. And I thought that was RONSLOW's opinion too. Look at RONSLOW's and my first comments again.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560288
Mike
>> I was doing everything 'right'.  
If you did it right, it wouldn't crash.

Exactly .. the thing I did WRONG was follow yours and jahnce's advice.  Thanks for agreeing that your method is flawed yet again.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560297
Jhance:

>"Is a class derived has a public base class base, then a pointer to derived
>can be assigned to a variable of type pointer to base without use of explicit
>type conversion.  The opposite conversion, for pointer to base to
>pointer to derived, must be explicit. >For example:
>class base {....};
>class derived : public base {...};
>
>derived m;
>base *pb = &m;    // implicit conversion
>derived *pd = pb; // error: a base * is not a derived *
>pd = (derived *)pb; // explicit conversion
>
>Hmmmm that last line looks a lot like:
>
>CComboBox *c = (CComboBox *)GetDlgItem(...);

but it is NOT the case .. gees .. don't you READ what I've posted.  Or do you fail to understand.

In the example you quote there is an object of type 'derived', and a pointer-to-base' pointer.  That is fine.  I even quoted that example right at the start of all this.

But that is NOT what is happening with GetDlgItem.  it is EXACTLY equivalent to

class base {....};
class derived : public base {...};

base m; // note the differnec
base *pb = &m;    // implicit conversion
derived *pd = pb; // error: a base * is not a derived *
pd = (derived *)pb; // explicit conversion

This is WRONG. becuase now you have a pointer-to-derived that is actually pointing to an instance of the base class.

So you example is NOT AT ALL REVLEVANT TO WHAT IS BEING DISUCSSED.

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560298
jhance.

>You must be doing something wrong.  I tried this and it works fine.  Maybe you need to cook up a phony example that crashes better

How DARE you suggest I cooked up a phony example.

Those three lines were enough to make a simple app crash.

Guies you are too incompetent to reproduce it (must be in DEBUG mode so that the corruption is detected.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560309
>Evidence in my favor:
>
>MS MFC docs, Brain, Blaszczak, MFC source code, numerous MFC SAMPLE apps all show this being used.

The particular examples given are 'safe' bewcuase

>Plus a reference from some guy from AT&T labs about this being a valid C++ technique as well.

No such thing .. your' example is for a DIFFERENT CASE ALL TOGETHER

>Evidence in your favor:

>DiLascia's magazine column and an example using CCheckListBox that
>doesn't crash.

It DOES crash .. want me to send you the error messages and source code to reporudce if you are unable to do so yourself?

>If we were in court, you'd lose
because you haven't proved your point
>by a preponderance of the evidence.

Bulshit

>Even your one-time supporter chensu
seems to have backpedaled:

"My opinion:

Use this technique with extreme
caution only when you know what you
are doing. Use DDX/subclass whenever
possible."

He is AGREEING with me.

OK .. here is the bottom line

The only time you can do the cast from GetDlgItem and have it not corrupt something somewhere is:

1) You are not calling a virtual function defined in the derived class
2) You are not calling a function that references a member variable of the base class
3) You are not calling a function that indirectly calls a function that fails on count 1 and 2
4) The functions called do not check the object type by either RTTI or MFC runtime-class checking
5) The compiler or runtime library do not check pointers for you
6) You can guarantee that NONE of the conditions will change in the future.

Now .. for the CURRENT implementation of MFC, there are SOME classes (CEdit, CListBox, CStatic, CButton, CComboBox)and some member functions you can call and have this work.  That does NOT mean (as you and mike have done), that you can say you can safely and legally cast to ANY CLASS DERIVED FROM CWND.  This is just simply not true and easily demonstrated (my simply application that you were unable or unwilling to reproduce).

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560321
I have had a consistent line through this whole discussion.  The tehcnique is bad C++ coding.  It happens to work in some situations.  It does not work in general.  It does not work for all CWnd* derived control classes.

I have yet to be proved wrong on any of these points.  And Mike has (reluctantly) agreed with me on all these points.

jhance and mike keep changing there asseerttions:  First it is "perfectly acceptable, legal C++", then they admit it is pedantically wrong and that a general and more obvious equivalnet exmaple is "not applicable".  Next they say it "is legal for ANY CWnd derived class in an MFC application", then admit that "There are a few control classes which can't be 'bad polymorphed'. CRichEditCtrl, for instance, and CCheckedListBox".  And mike agres that you should "Use this technique with extreme caution" .. why would that be it is a safe technique?

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560325
BTW: I may not be able to contribute here for a day or two .. it is the weekend now.
0
 
LVL 5

Expert Comment

by:Wyn
ID: 2560376
Sorry to break in...
Just a simple question:

WHY don't MICROSOFT make GetDlgItem()  dynamically check the control type and return the correct one rather than return a CWND* ?

I think this will appease all fuss here.

Regards
W.Yinan  
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560379
>Don't believe me?  See for youself:
ok .. lets have a look

DOCKSTAT.CPP:

CDockBar* pDockBar = (CDockBar*)pDockFrame->GetDlgItem(AFX_IDW_DOCKBAR_FLOAT);

The dockbar already has a CDockBar attached to it, so GetDlgitem returns a pointer to a CDockBar object, not a CTempWnd object.
1 point for me

DOCMGR.CPP:

CListBox* pListBox = (CListBox*)GetDlgItem(AFX_IDC_LISTBOX);

CListBox is one of the few CWnd derived clases that work.
2 points for me
1 point for you (I'm generous)

PPGCOLOR.CPP:

SetButton((CColorButton *)(GetDlgItem(CColorButton::idClicked)));

the CColorButton was subclassed beforehand (just like DDX_Control does).  This is fine (becuase you get a pointer to a CColborButton object
3 points for me
1 point for you

PPGFONT.CPP:

GetDlgItem(AFX_IDC_STRIKEOUT)->EnableWindow(FALSE);

GetDlgItem(AFX_IDC_UNDERLINE)->EnableWindow(FALSE);

There is no cast here, so no problem.  We are calling CWnd functions
4 points for me
1 point for you

PPGPICT.CPP:

(Oh No!  It's our old friend the CComboBox!!!!!)

CComboBox* pPropName = (CComboBox*)GetDlgItem(AFX_IDC_PROPNAME);

Yes .. combo box is ok .. as long as you don't call any of its virtual functions
5 points for me
2 point for you (I'm generous again)

VIEWCORE.CPP:

return (CScrollBar*)pParent->GetDlgItem(nIDScroll);

Scrollbar is another of the 'safe' subclasses (no members no virtuals)
6 points for me
3 point for you (I'm generous again)


VIEWPREV.CPP:

CPreviewView* pView = (CPreviewView*) pFrameWnd->GetDlgItem(AFX_IDW_PANE_FIRST);

Afgain, the view is attached to a CPreviewView object, so the return really is a pointer to a CPreviewView object.  This is easily seen by the line that follows it
  ASSERT_KINDOF(CPreviewView, pView);
If you tried the corresponding statemtn in your CComboBox exmaple (with no DDX_Control) then the assert would FAIL because it would NOT be pointing to a CComboBox object.

7 points for me
3 point for you

WINFRM2.CPP:

CDockBar* pDockBar = (CDockBar*)pDockFrame->GetDlgItem(AFX_IDW_DOCKBAR_FLOAT);

Same as above.
7 points for me
3 point for you

>Better luck next time.....

exactly my thought.  Your example have done nothing to prove your case, and indeed support mine.

Oh dear.

SIDE ISSUE: If even MFC were made more safe by augmenting the existing tests in CComboBox functions such as
  _AFXWIN_INLINE int CComboBox::GetCount() const
    { ASSERT(::IsWindow(m_hWnd)); return (int)::SendMessage(m_hWnd, CB_GETCOUNT, 0, 0); }
by adding the (quite reasonable):
  ASSERT_KINDOF(CComboBox, this);
then all your existing code that casts GetDlgitem without attaching or subclassing would FAIL !!  Lets hope the MFC designers never do that, eh?

0
How to run any project with ease

Manage projects of all sizes how you want. Great for personal to-do lists, project milestones, team priorities and launch plans.
- Combine task lists, docs, spreadsheets, and chat in one
- View and edit from mobile/offline
- Cut down on emails

 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560390
>WHY don't MICROSOFT make GetDlgItem()  dynamically check the control type and
>return the correct one rather than return a CWND* ?

Good question.

If there is already a CWnd-derived object attached to the window handle, then it does.

If not, then it would need to know what class is associated with the control.  Guess it could look at the window-class and if it is one of the predefined ones (like "EDIT" for an edit control), then it could create a temporary window of the correct type (CEdit in this case).

That would help a little, as long as there is only one CWnd-derived class that can be associated with that type of window.  However, sometimes there is more than one.  And there could be user-defined CWnd-derived classes one wants to use.

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560399
Part of the problem is that the GetDlgItem function must be declared to return SOMETHING (cannot change the type of pointer).

But that is not the real problem.  The problem is not the type of the pointer.  It is the actual type of the object being pointed to.

If there is no CWnd-derived class attached to the window, GetDlgItem returns a pointer to a CTempWnd object created in the temporary window map. It disappears after a while.  And that CTempWnd object does not have the member variables or virtual functions declared in any other CWnd-derived class.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560487
To clarify... the function must be declared as returning a particular type eg

CWnd* GetDlgItem(int id);

So the return type is a pointer.  That is a (current) limitation of C++.  However, a CWnd* can point to an object of any type dervided from CWnd.  If you KNOW the type of object being pointer to, then you can safely and legally cast the CWnd* returned to a CWhatever*.  No problem.  And this is the case when a CWnd-derived object has been Attach'd to the window handle in GetDlgItem.

However, if the object being pointed to is of type CWnd (as when you haven't subclassed/attached to a CWnd-derived class), then casting the return to to a pointer to some other type derived from CWnd is bad C++ code, that may happen to "work" in some particular circumstances (as described in earlier messages from me).


0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2560579

 > The dockbar already has a CDockBar attached to it, so
 > GetDlgitem returns a pointer to a CDockBar object, not
 > a CTempWnd object.

 > 1 point for me

I guess I don't understand your position, RONSLOW. Is casting the return value from CWnd::GetDlgItem() improper, or not? Or is it only improper when you can't prove what the return type is?

Or, maybe I just don't understand your scoring.

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560610
jhance:

Just found out why you wouldn't have gotten an error andI did.

I use BoundsChecker when running debug apps.  It detects memory access problems etc.  When BoundsChecker is off, the program doesn't exhibit a crash .. most likely because code that allocates memory for the CTempWnd in GetDlgItem deliberately turns off the MFC memory diagnostics so there is no checking of the memory.

That means when the sample code overwrites memory outside the allocated area, you don't get a crash.

In any case, the app is STILL doing the wrong thing by writing outside the allocated memory.  And it could well stuff up other things in the program (as memory overruns are want to do).

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560619
Mike:

>I guess I don't understand your position, RONSLOW. Is casting the
>return value from CWnd::GetDlgItem() improper, or not? Or is it only
>improper when you can't prove what the return type is?

It is quite simple.

If we know that the window has been attached to a CWnd-derived class (eg DDX_Control, or Attach, or was created via the CWnd-derived object) then there is no problem, because the value pointed to by the pointer returned from GetDlgItem is an object of the derived class.  I have NEVER said that casting in this case is wrong.

However, if the window has NOT been attached to a CWnd-derived class, then GetDlgItem will create a temporary CTempWnd object and return a pointer to that.  If you app then casts the pointer to be a pointer-to-derived-class, and dereferences it, then this is not good C++, and in the cases described (such as CCheckListBox where there are additional member data) we get memory access violations etc. that can corrupt data etc.

My scoring is, if the example shows a case that under my explanation would be correct, I get a point.  If it shows an example where unsafe code is used, then jhance+mike get a point.  If it is a special case (like casting to CEdit* under current MFC implementation), then we both get a point (both of us right).
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560723
I just don't believe this .. you're rejecting good and legal C++ coding in favour of lazy code and possible program crashes.

This whole discussion was a farce .. You never intended to listen or take the arguments seriously.

I have little or no respect for you or Mike now after this fiasco.  What a crock.

You were unable to prove your invalid argumetns correct and kept changing your claims to suit as each point was shown wrong.

Stuff you
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2560728
>I'll award the
points to whoever offers the best
>explanation of why RONSLOW is right

I did .. the first comment quoted from Paul.  You just ignored that.

>Further, if the balance of opinion is that RONSLOW is correct, I'll also post
>another 200 pt question for him as a reward for being right.

Yeah .. right.  As if you even intended to.  You'd alrady made up your mind .. and a few facts weren't going to get in your way.

I hope your applications DO blow up in your face .. it would serve you right.

BTW: You think I am pissed of ... you are right !!!
0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2561109
RONSLOW> How DARE you suggest I cooked up a phony example.  [...]
 RONSLOW> Guies you are too incompetent to reproduce it (

That's it: this has gone from "not really worthwhile" to "counterproductive".  Good luck with your research.

 RONSLOW> I have little or no respect for you or Mike now after this fiasco.  

Huh?  What the heck did _I_ do?

..B ekiM
0
 
LVL 23

Expert Comment

by:chensu
ID: 2561364
I don't really see any major difference between RONSLOW's opinion and mikeblas's opinion. The only difference is that RONSLOW tends to not use this technique while mikeblas tends to use this technique.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561760
Sorry .. I was tried and very frustrated with the seeming brickwall I was trying to convince.

I'm feeling much better after my vent and a good nights sleep.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561794
OK .. lets try to consolidate here. Maybe we can agree (and not even agree to differ).
0
 
LVL 2

Expert Comment

by:tdubroff
ID: 2561927
Wow.  Compared to you guys I'm new to coding in C++ and MFC, but this was very enlightening.  I think, at least it was evident to me, that RONSLOW has proved his point.

I really really think that this discussion should be consolidated into an article for Visual C++ Developer's Journal or some such magazine.  Any hope of that?  In the meantime, I'm going to send this URL to my buddies at work.

-Ted
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561937
Hopefully we can all agree on this:

1) What is/isn't good, valid and legal C++ (in general and in 'theory').  Given:
  class Base {};
  class Derived : public Base {};
and
  Base b;
  Derived d;
  Base* pB;
  Derived* pD;
These things are fine:
  pB = &b;
  pD = &d;
  pB = &d; // implict upcast
  pD = (Derived*)pB; // ok as pB pointer to Derived object d
But this is not fine
  pB = &b;
  pD = (Derived*)pB; // bad as pB pointer to Base object b;

2) What you can get away with (on most platforms, even though it is theoretically wrong) assuming we have
  pB = &b;
  pD = (Derived*)pB; // bad as pB pointer to Base object b;
this can be ok IFF
*  Derived does not define any memeber variables which are accessed/modified via pD or calls through it
*  Derived does not define any new virtual functions which are called directly of indirectly via pD
*  You do not call any virtaul functions ofBase that are overriden by Derived
*  You do not use runtime type checking, typeof, dynamic_cast etc on pD

Doing any of these things can result in memory corruption, overwrites, illegal addresses etc.

Hopefully we all agree on this?
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561956
Part 2 ...

After agreeing to that let slook at GetDlgItem (and the guts of MFC under that).

1) GetDlgItem is declared as returning a CWnd*.  The actual object pointed to by the CWnd* will be either a CWnd or CWnd-derived

2) If the corresponing window is attached to some CWnd-derived (or sometimes CWnd) object, then that attachment is registered in the permanent handle map and GetDlgItem returns a pointer to the actual CWnd-derived object.

3) An entry is put into this map is the object is attached to the window handle, the window is created via the object (Create call) or it is subclassed (often via DDX_Control).

4) If there is no entry in the permanent map for a window handle, then GetDlgItem will create a new temorary CTempWnd and stick that in a temporary map and return a pointer to a CTempWnd.  This is usually the case with dialog control when you don't use DDX_Control and use GetDlgItem instead

5) If you cast the return value of GetDltItem (in case 4 above) to a CWnd-derived class (like CCheckListBox) then you have the bad C++ coding described in the previous message.  You can only "get away with this" if you satify the conditions mentioned before.

6) The only classes that you can cast to (in MFC) and still have 'working' code are (with the current implementation of MFC):
  CEdit, CStatic, CButton, CListBox, CComboBax, CScrollBar
and, through the cast pointer, you should not call any virtual functions of these classes nor of CWnd.

7) Only in these cases (a handful of MFC classes and a subset of their member functions) will the code "work" and only with the current implementation.  One cannot gurantee that there wont be new member vars, virtual functions, runtime class checking etc added in subsequent MFC releases.

8) It is therefore NOT correct to state that you can always cast the return value of GetDlgItem to a pointer to ANY CWnd-derived class (and use that pointer) with safety.  There are cases where this will not work and may be more cases in the future.

9) [the controversial bit] putting this forward as a general techinique without the warnings given is not responsible behaviour, and if such recommendations ARE made, it is reasonable to warn readers that this is NOT a good general technique and only works with certain classes.

Anyone honestly disagree with ANY of this??

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561958
Part 2 ...

After agreeing to that let slook at GetDlgItem (and the guts of MFC under that).

1) GetDlgItem is declared as returning a CWnd*.  The actual object pointed to by the CWnd* will be either a CWnd or CWnd-derived

2) If the corresponing window is attached to some CWnd-derived (or sometimes CWnd) object, then that attachment is registered in the permanent handle map and GetDlgItem returns a pointer to the actual CWnd-derived object.

3) An entry is put into this map is the object is attached to the window handle, the window is created via the object (Create call) or it is subclassed (often via DDX_Control).

4) If there is no entry in the permanent map for a window handle, then GetDlgItem will create a new temorary CTempWnd and stick that in a temporary map and return a pointer to a CTempWnd.  This is usually the case with dialog control when you don't use DDX_Control and use GetDlgItem instead

5) If you cast the return value of GetDltItem (in case 4 above) to a CWnd-derived class (like CCheckListBox) then you have the bad C++ coding described in the previous message.  You can only "get away with this" if you satify the conditions mentioned before.

6) The only classes that you can cast to (in MFC) and still have 'working' code are (with the current implementation of MFC):
  CEdit, CStatic, CButton, CListBox, CComboBax, CScrollBar
and, through the cast pointer, you should not call any virtual functions of these classes nor of CWnd.

7) Only in these cases (a handful of MFC classes and a subset of their member functions) will the code "work" and only with the current implementation.  One cannot gurantee that there wont be new member vars, virtual functions, runtime class checking etc added in subsequent MFC releases.

8) It is therefore NOT correct to state that you can always cast the return value of GetDlgItem to a pointer to ANY CWnd-derived class (and use that pointer) with safety.  There are cases where this will not work and may be more cases in the future.

9) [the controversial bit] putting this forward as a general techinique without the warnings given is not responsible behaviour, and if such recommendations ARE made, it is reasonable to warn readers that this is NOT a good general technique and only works with certain classes.

Anyone honestly disagree with ANY of this??

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561962
tdubroff: Thanks for the support.  Shame it was too late to actualyl make a difference .. jhance had already decided he was right and I was wrong.

PS: Sorry for the double post abive.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2561967
jhance .. do you care to change your mind and offer me the 200 points you promised?

I think I've successfully argued my case .. unless there is really anything in the above summary messages that you seriously care to disagree with?

I await my reward :-)

0
 
LVL 23

Expert Comment

by:chensu
ID: 2562049
Agree all.

A dog is an animal. A pointer to a dog can be safely cast to a pointer to an animal since you can ask a dog to do what an animal does. But casting a pointer to an animal to a pointer to a dog is not safe. An animal may not be able to do what a dog does and may not have what a dog has. Of course, it is no problem if the animal is indeed a dog.
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2562339
woof
0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2563002
It's great to  study and discuss this issue. But repeatedly taking the matter to an offensive and personal level as RONSLOW has is reprehensible.

By getting so emotionally involved, I think RONSLOW has significantly overstated the severity and history of the issue at hand.

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2563207
>It's great to study and discuss this issue.

What also makes it less enoyable is the complete dismissal of the reasons based on sounds coding practice and of examination and explantion of the MFC internals (by jhance in particular).

>But repeatedly taking the matter to an offensive and personal level
>as RONSLOW has is reprehensible

It was quite an enjoyable and (I thought) firendly disucssion until I was acused by jhance of cooking up phony examples.  If my integrity and honesty is attacked .. I fight back .. if that is the way people want to play, then I'll play that way too.  But I'd certinaly not describe my posts as repeatedly offsense and personal !!  Would anyone else?

So please, don't going singling me out for reprehensibility without similar comments about jhance who (to be childish for a moment) started it.

BTW: in case you missed it, I've already apologised for my one late-night tired and frustrated outburst.

>By getting so emotionally involved, I think RONSLOW has significantly
>overstated the severity and history of the issue at hand.

What is the 'overstated history' you are referring to?

In any-case, any personal issues aside, I am still perfectly correct in my proof that jhances contention is WRONG.

And I also still feel that recommending potentially dangerous and technically incorrect techinques without a warning or explanatiaon is irresponsible.  Do you feel otherwise?

In summary, I think he definitely owes me a 200pt question for proving that his contention was wrong.  And maybe even an apology for calling it baloney.
0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2563378
> (by jhance in particular).

That's between you and jhance, I guess. I don't see why you've dragged me into it.

 > firendly disucssion until I was acused by jhance of cooking
 > up phony examples.  

And, in return, instead of explaining your example, you told jhhance he was incompetant. That's offensive. And unproductive. After that, you admitted that you figured the reason it didn't crash for jhance was that you're running BoundsChecker and he might not be. Of course, you didn't apologize for not explaining that first.

Purporting that some part of MFC's design was an accident? I've read few things so pompous.

Then, you told me you've lost complete respect for me. What's that suppose to prove?

 > already apologized

Apologized to whom? For which outburst? I see that posting; it doesn't seem very contrite.

Me, I had a real hard time latching on to your argument. Because of all the heat you were radiating, I couldn't see any light. To me, it sounded like you were arguing the more academic aspect of this issue--that downcasting the CWnd* pointer is a bad design. That design exactly matches the SDK implementation of GetDlgItem(), though. A HWND that comes back is actually of a given type and only supports certain operations, and you need to know what it is before you can access its user data, or even reliably send it messsage.

Folks occasionally argue that, but I think it's a pedantic argument.

 > What is the 'overstated history' you are referring to?

If you're just interested in pointing out the rules, that's great. The rules do exist, and aren't commonly expressed because they're not easily described. Knowing these rules can help. But using them isn't an indication of a developer being being "lazy", or even being "WRONG".

 > So please, don't going [sic] singling me

Frankly, jhance seems pretty skeptical to me. Maybe too skeptical, but I that's better than instantly believing any of the random bunk that floats around the programming communities these days.

Anyhow, I don't see him as being nearly as abrasive as you are.

 > 6) The only classes that you can cast to (in MFC) and still have
 > 'working' code are (with the current implementation of MFC):

What about CListCtrl, CTreeCtrl, and CSpinButton? What about CHeaderCtrl? And CIPAddrCtrl?

 > Maybe an apology

Maybe. But, again, that's between you and jhance.

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2563437
>> (by jhance in particular).
>That's between you and jhance, I guess. I don't see why you've dragged
>me into it.

My apologies if you feel you've been dragged into this .. and, yes, I got a bit heated and even took it out on you (and I did apologise afterwards - but do so again here).

>And, in return, instead of explaining your example, you told jhhance he was
>incompetant

I had ALREADY explained why the example should crash.  Upon deaf ears it appeared.  And after Jhance failed to reproduce the crash on his system, rather than requrest for more info, he accused by of cooking up a phony example.

>Of course, you didn't apologize for not explaining that first

Oops .. a genuine mistake.  I had meant to apologise in that message.

>> already apologized
>Apologized to whom?

To you:

>For which outburst?

The one in preceeding messages where I said "I have little or no respect for you or Mike now after this fiasco." to which you asked what had you done?

>I see that posting; it doesn't seem very contrite.

It was.  It was something I shouldn't have been thinking let alone typing.  The apology is certainly sincere.

>What about CListCtrl, CTreeCtrl, and CSpinButton? What about CHeaderCtrl? And CIPAddrCtrl?

Oops. Missed them .. only looked at the ones in heirarchy defined in AfxWin.h

Then, of course, one should add to the list of one sthat don't work:incomp
CDragListBox, CToolTipCtrl

Still .. blanket advice that it is always ok to cast to any CWnd* derived class is bad.  One needs to look at the declaration (and implementation) of the class before one can decide that it is ok for the current MFC release.

>Anyhow, I don't see him as being nearly as abrasive as you are.

I did't become "abrasive" until the reply to the "cook up phony examples" remark.

Guess it comes down to ones own opinion.  If you feel I am abrasive, I'm sorry.  But you are entitled to that opinion.  I guess I tend to overreact a little (;-) when I feel I am under personal attack.  I should learn to handle it better.

BTW: I have also heard some unkind remarks about yourself in the past .. usually people confusing your sense of humour with bad manners.  Personally I don't find your remarks offensive - you even 'have a go' at yourself on occasion :-)

Anyway, I am genuinely sorry for any offense.

0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2563659
> but do so again here).

Thank you.

 > The one in preceeding messages where I said "I have little
 > or no respect for you or Mike now after this fiasco." to
 > which you asked what had you done? [...] certainly sincere.

Thank you; that's nice to read.

 > usually people confusing your sense of humour with bad manners

Yeah, it happens a lot. When the finishing school found out I had been asked to leave the tennis club again, I found myself looking forward to a long life of ill-refinement... and all the tedious explanations that go with it.

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2563668
You're welcome.

I'm usually a really nice guy (even if I'm not very good at tennis).

Really.

0
 
LVL 7

Expert Comment

by:KangaRoo
ID: 2564339
>> Namely that C++ maintains the vtbl order of functions for dervied classes.
It doesn't. There is not even a mentioning of vtables in C++, let alone that is says anything
0
 
LVL 11

Expert Comment

by:mikeblas
ID: 2566539
It's already happening: people who read this thread and have an app that crashes are jumping to the conclusion that it crashes becaue they used GetDlgItem().

Why doesn't anybody debug anymore?

..B ekiM
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2566915
Good question.

Debugging is becoming a lost art.  We are all taught how to do the analysis and design and how to write the program.  But debugging?  No one seems to teach that .. it seem more of a black art than a science.

Of course, it is still important to be better able to write correct code in the first place.  Prevention being better than cure.  But when the inevitable bugs still happens, it is too easy to shout "MFC bug!!", or go looking for excuse somewhere else beofer understanding the problem.

However, at least developers now have another area of _possible_ problem that is worth eliminating BEFORE it happens and something else to check for if an obscure problem occurs.

If it IS an actual problem, then people reading this article will have an understanding (I hope) of WHY it is happening and some things that can be done, with little or no performance hit, to make it work.

If they are writing new programs, they can avoid obscure problems in the first place by avoiding the casting technique when it is not appropriate.

If it weren't for this issue being raised, they'd be looking at their VC/MFC books and says "well, it says here I can do it, but when I add this line my program crashes .. what is going on?"

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2567094
Mike has posted elsewhere what is probably the best 'why it is over-reacting' reason.

Basically, if the class you are casting to is 'value-added' (eg. like CCheckListBox that adds checkboxes to your list control), then you will probably already have a CCheckListBox object around that you attached to the control so that you'd see the desired effect.  So any cast the CCheckListBox from GetFlgItem would be ok.

The counter to this is that if you did this, you wouldn't use GetDlgItem anyway, because you'd just use the existing CCheckListBox object.

But the gist of Mike's argument apeears to be that it is most likely that the classes, to which you will tend to cast the return value from GetDlgItem, will be the basic MFC-supplied CWnd-derived control classes that provide no extra functionality.  These are the ones for which this technically/theoretically bad technique does "work" in practice.

I still think the template technique I put forward is a safer solution for the future and for any other classes.  It doesn't about the same amount of processing as a plain GetDlgItem does (if not less) and at least is valid C++ coding (doesn't break any 'rules').

NOTE: I've also revise the design a little now so that you can use these as member variables of the dialog class, giving you that advantages of DDX_Control with even less work and performance overhead.  Might publish this somewhere with an article one day soon.

0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2567102
Mike has posted elsewhere what is probably the best 'why it is over-reacting' reason.

Basically, if the class you are casting to is 'value-added' (eg. like CCheckListBox that adds checkboxes to your list control), then you will probably already have a CCheckListBox object around that you attached to the control so that you'd see the desired effect.  So any cast the CCheckListBox from GetFlgItem would be ok.

The counter to this is that if you did this, you wouldn't use GetDlgItem anyway, because you'd just use the existing CCheckListBox object.

But the gist of Mike's argument apeears to be that it is most likely that the classes, to which you will tend to cast the return value from GetDlgItem, will be the basic MFC-supplied CWnd-derived control classes that provide no extra functionality.  These are the ones for which this technically/theoretically bad technique does "work" in practice.

I still think the template technique I put forward is a safer solution for the future and for any other classes.  It doesn't about the same amount of processing as a plain GetDlgItem does (if not less) and at least is valid C++ coding (doesn't break any 'rules').

NOTE: I've also revise the design a little now so that you can use these as member variables of the dialog class, giving you that advantages of DDX_Control with even less work and performance overhead.  Might publish this somewhere with an article one day soon.

0
 
LVL 6

Expert Comment

by:snoegler
ID: 2710111
CCheckBox cBox;

cBox.Attach(GetDlgItem(IDC_CBOX1)->GetSafeHwnd());
cBox.xxx();
cBox.Detach();

Just came to my mind. Is a bit slow, but should avoid the prob (at least the one with the vtable and/or memory corruption)

Maybe i repeat something which has already been posted. Sorry then :)
0
 
LVL 10

Expert Comment

by:RONSLOW
ID: 2710352
That will work, but, as you said, would be slower because it would need to attach (which adds to permanent handle map), and detach (which removes), every time it is needed.

If you are only doing this once in a while (say, in OnInitDialog), then that is probably a small price to pay.  If you want to get at the same item repeatedly, then use a member var that subclasses the control (DDX_Control is the easiest way to do this, or call SubclassDlgItem).

The problem with what you propose is if there is ALREADY a CChexkBox (or whatever) attached, then this wil stuff it up (as there can only be one CWnd-derived object attached to a given hwnd at a time).

The template class I wrote (see above somewhere) does the same thing as you propose, but is safer because it looks to see if there is already a CWnd-derived object attached (in which case it returns a pointer to it), otherwise it creates one dynamically and detaches and deletes when finished.
0

Featured Post

IT, Stop Being Called Into Every Meeting

Highfive is so simple that setting up every meeting room takes just minutes and every employee will be able to start or join a call from any room with ease. Never be called into a meeting just to get it started again. This is how video conferencing should work!

Join & Write a Comment

With most software applications trying to cater to multiple user needs nowadays, the focus is to make them as configurable as possible. For e.g., when creating Silverlight applications which will connect to WCF services, the service end point usuall…
For most people, the WrapPanel seems like a magic when they switch from WinForms to WPF. Most of us will think that the code that is used to write a control like that would be difficult. However, most of the work is done by the WPF engine, and the W…
This video will show you how to get GIT to work in Eclipse.   It will walk you through how to install the EGit plugin in eclipse and how to checkout an existing repository.
This is Part 3 in a 3-part series on Experts Exchange to discuss error handling in VBA code written for Excel. Part 1 of this series discussed basic error handling code using VBA. http://www.experts-exchange.com/videos/1478/Excel-Error-Handlin…

747 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

10 Experts available now in Live!

Get 1:1 Help Now