Closed
Bug 100743
Opened 23 years ago
Closed 22 years ago
CStringArrayToCharPtrArray::Convert has same code for if/else
Categories
(SeaMonkey :: MailNews: Address Book & Contacts, defect)
SeaMonkey
MailNews: Address Book & Contacts
Tracking
(Not tracked)
VERIFIED
INVALID
People
(Reporter: jag+mozilla, Assigned: cavin)
References
()
Details
Attachments
(3 files)
466 bytes,
patch
|
Details | Diff | Splinter Review | |
412 bytes,
patch
|
Details | Diff | Splinter Review | |
715 bytes,
patch
|
jag+mozilla
:
review+
|
Details | Diff | Splinter Review |
52 if (copyElements == PR_TRUE) 53 (*returnPropertiesArray)[i] = 54 array[i]->ToNewCString(); 55 else 56 (*returnPropertiesArray)[i] = NS_CONST_CAST(char *, (array[i]->ToNewCString())); I'm guessing that the |else| part of this shouldn't be copying, so a ->get() instead.
Comment 1•23 years ago
|
||
Reporter | ||
Comment 3•23 years ago
|
||
Also see line 119 for the Unicode version of it. John Marmion: - (*returnPropertiesArray)[i] = NS_CONST_CAST(char *, (array[i]->ToNewCString())); + (*returnPropertiesArray)[i] = NS_CONST_CAST(char *, NS_STATIC_CAST(const char *, *(array[i]))); Instead of that static cast, do this: (*returnPropertiesArray)[i] = NS_CONST_CAST(char *, array[i]->get());
Comment 4•23 years ago
|
||
The two lines are functionally the same. You suggest the get() method while I was making use of the operator const char * virtual const char* get() const (return mStr;} operator const char*() const { return (const char*) mStr;} But Yes, I agree with you. Yours is a simpler implementation. I will update the patch immediately. It needed to be updated following bug 100476 plus the new Licensing Header in the source files. We should look to get this checked in. Thanks for the input
Comment 5•23 years ago
|
||
Reporter | ||
Comment 6•23 years ago
|
||
Could you do the same for the PRUnichar code a little lower? And the reason I suggested get() instead of the implicit conversion (forced by your cast) is bug 53057.
Comment 7•23 years ago
|
||
Reporter | ||
Comment 8•23 years ago
|
||
Comment on attachment 52207 [details] [diff] [review] fix the fallout from #100476 r=jag
Attachment #52207 -
Flags: review+
Reporter | ||
Comment 9•23 years ago
|
||
Could you take a quick look at the code that calls this function with copyElements == PR_False and make sure they're not freeing the shared string they get?
Comment 10•23 years ago
|
||
Checked the calling code as requested. I am happy that this is OK.
QA Contact: nbaca → stephend
Comment 11•22 years ago
|
||
Cavin, this patch has been checked in as part of the fix for bug #116972. I suggest that this should be closed as not a bug i.e. INVALID.
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → INVALID
It has been, yes, and I've already verified that bug, too.
Status: RESOLVED → VERIFIED
Updated•20 years ago
|
Product: Browser → Seamonkey
You need to log in
before you can comment on or make changes to this bug.
Description
•