Re: Issue with casting CString to LPARAM and recasting it to
CString...
On Jun 13, 7:53 pm, Joseph M. Newcomer <newco...@flounder.com> wrote:
See below...
On Fri, 13 Jun 2008 04:42:19 -0700 (PDT), Padmalatha <padmalathagirid...@g=
mail.com> wrote:
Hi,
I have developed a custom control with some customizations of the
List Control class of MFC (CListCtrl). I have another custom class
(MyItem) which is inherited from CItem. Each and every entry in the
list control is a MyItem.
Every time a new item is added into the LIst control we use the logic
of maintaining a unique sequence number which is a CString and member
of MyItem class. Here goes the implementation of the 2 main members of
MyItem class
****
From the above, I presume that your declaration is
CString m_sequence;
****
void MyItem::SetListItemSequence(const CString& seq)
{
m_sequence = seq;
}
This member sets the newly added entries sequence value into the
member.
/////////////////////////////////////////////////////////////////////////=
/=AD/////////////
CString* MyItem::GetListItemSequence()
{
return &m_sequence;
}
This member gets the added Sequence numbers Address. The caller of
this function is a member from my customized list control class, we
call this while inserting the item into the list control and we call
it like this - we first call the GetListItemSequence() from the insert
method lieke this-
****
This looks suspicious. Why are you returning a pointer? I would be i=
nclined to do
CString MyItem::GetListItemSequence()
{
return m_sequence;
}
because returning a pointer can be dangerous...for example, it allows the =
caller to reach
in and modify the contents of the CString in arbitrary ways, while making =
a copy
guarantees the integrity of the string
****
SetItemSequence(nRow,(LPARAM)(LPCTSTR)item.GetListItemSequence());
****
I'd be disinclined to write code like this; I'd rather define it as
SetItemSequence(nRow, (LPARAM)new CString(item.GetItemSequence()));
That way, you are not depending on some unknown lifetime of the sequence n=
umber string.
This reduces risks signficantly. I believe that when you have an LPARAM=
, the control
itself is in complete control of the LPARAM value, and it is not managed b=
y anyone else.
In this case, the validity of the pointer critically depends on the fact t=
hat the sequence
number integrity is maintained.
In fact, this code cannot possibly work! Now that I look at it, I reali=
ze that you are
always giving a pointer to the address of where the sequence number CStrin=
g *was*, which
means that each time you assign a new CString, you invalidate the pointer.=
And if you
don't invalidate the poitner, the pointer is pointing NOT to the current s=
tring but to
some other random string.
DO NOT depend on pointer integrity like this; it is a losing idea. The =
chances that the
pointer will ever make sense border on zero, and even if the pointer remai=
ned valid, you
have, in effect, a pointer to a variable which is shared by ALL instances =
of the LPARAM.
Write the code in the simplest possible fashion, and that means NOT sharin=
g pointers like
this!
****
here row is the row number of the newly added list entry. and item is
the object of MyItem class. So calling the MyItem's
GetListItemSequence() which actually returns the address of the
sequence number. Converting this into LPARAM like shown above. Is this
a issue?????? That SetItemSequence() is a member of MyListControl
class which takes the address and sets the row sequence with the type
casted LPARAM value.
void MyListControl::SetItemSequence(int item,LPARAM lParam)
{
LVITEM pItem;
InitLVITEM(item,0,&pItem);
LS_item *lpLS_row = (LS_item*) pItem.lParam;
if(lpLS_row)
{
lpLS_row->pSequence = lParam;
****
Why is this value an LPARAM type and not a CString? Why is the second a=
rgument an LPARAM?
Why are you forcing the CALLER to do the casting, when it should be done h=
ere (make the
second parameter a CString, and instead of doing
(LPARAM)new CString(item.GetItemSequence())
just have teh caller write
item.GetItemSequence()
which is a CString, and you do the ugly casting, if required at all, INSID=
E the function
that handles it!
****
}
}
Now my isssue here is - when ever the user double clicks on any
particualar item i will have to show him the contents of the item. So
I need to know the sequnce number so i implemented another member like
this-
CString CListCtrlStyled::GetItemSequence(int item)
{
CString retVal;
LPARAM rc = 0;
LVITEM pItem;
InitLVITEM(item,0,&pItem);
LS_item *lpLS_row = (LS_item*) pItem.lParam;
if(lpLS_row)
{
if (lpLS_row->pSequence)
retVal = *(CString*)(lpLS_row->pSequenc=
e);
}
return retVal;
}
Now this function works fine almost all cases. But it does crash
sometime and it crashes right at the place where the deferencing is
happening. I see that the adress value i.e. lpLS_row->pSequence is
fine. (checked this using log files) but the content may b is bad or
somthing. CString conversion code crashes as it finds the srcString is
"".(empty ??? may b?? not sure). Can any one please help me out. I am
blown off with the ideas. If it fails at every instance I can fix this
may b :) but it crashes once in a while. I feel there is some type
casting issue with CString to LPARAM and later dereferencing it. Any
help will b more than thankful.
****
Yes. Given the fundamental design error of not creating a string manage=
d exclusively by
the control, I'm surprised it doesn't ALWAYS crash. In looking at the c=
ode, it is more
amazing that the phrase you can use is "sometimes" and not "always". Th=
e code is just
wrong. Fix it as indicated.
(One of the first and best tests of erroenous design in C++ is the random =
passing around
of pointers. Just make a copy of the string and use that in LPARAM, and=
have a DeleteItem
handler for your control that deletes the string when the item is deleted.=
Or, more to
the point, why are you not letting the control manage the entire item in i=
ts LPARAM...that
could be another problem. In any case, distributed management like this=
is dangerous, and
in this case you don't have to do it)
joe
****
Joseph M. Newcomer [MVP]
email: newco...@flounder.com
Web:http://www.flounder.com
MVP Tips:http://www.flounder.com/mvp_tips.htm- Hide quoted text -
- Show quoted text -- Hide quoted text -
- Show quoted text -- Hide quoted text -
- Show quoted text -
Hi,
Thanks a lot for all your ideas. I replaced the LPARAM conversions
fully from my code and using CString directly and things r solved now.
I didnot get the issue in a days unit testing. Thanks again guys.
Cheers
Padma