100743 - CStringArrayToCharPtrArray::Convert has same code for if/else

Status: VERIFIED INVALID

(SeaMonkey :: MailNews: Address Book & Contacts, defect)

Description

[jag (Peter Annema)]

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

[John Marmion]

Attachment: One line patch fix

Comment 2

[scottputterman]

reassignig to cavin

Comment 3

[jag (Peter Annema)]

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

[John Marmion]

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

[John Marmion]

Attachment: use the get() plus keep the patch up-to-date against the head

Comment 6

[jag (Peter Annema)]

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

[John Marmion]

Attachment: fix the fallout from #100476

Comment 8

[jag (Peter Annema)]

Comment on attachment 52207 [details] [diff] [review]
fix the fallout from #100476

r=jag

Comment 9

[jag (Peter Annema)]

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

[John Marmion]

Checked the calling code as requested. I am happy that this is OK.

Comment 11

[John Marmion]

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.

Comment 12

[[e-mail] (gone - use [e-mail] instead)]

It has been, yes, and I've already verified that bug, too.