943870 - Consider removing char* casts from char16ptr_t.

Status: NEW

(Core :: XPCOM, defect)

Description

[Jacek Caban]

See bug 928351 comment 36:

> +    }
> +    explicit operator unsigned char*() const {
> +      return const_cast<unsigned char*>(reinterpret_cast<const unsigned char*>(ptr));
> +    }
> +    explicit operator void*() const {

These four const char*, const unsigned char*, unsigned char*, void* explicit operators are blargh.  How many instances were we actually hitting where these were necessary?  If it's relatively "few" I would probably go more for uglifying casts at those sites, or in some utility method in some central header that did the relevant sequence of casts instead.  (This would be followup-land, to be clear.)

Comment 1

[Biru [:poiru]]

Attachment: Remove explicit {char,void}* cast operators from char16ptr_t

Based on the successful try builds, it seems that we aren't using the casts at all: https://tbpl.mozilla.org/?tree=Try&rev=e2542e8ffd65

Comment 2

[Jacek Caban]

This code is not covered by try. It's mingw-specific.

Comment 3

[Biru [:poiru]]

(In reply to Jacek Caban from comment #2)
> This code is not covered by try. It's mingw-specific.

Oh, I see. I'll unassign myself then. Thanks for the info!

Comment 4

[(no longer active)]

(In reply to Jacek Caban from comment #0)
> See bug 928351 comment 36:
> 
> > +    }
> > +    explicit operator unsigned char*() const {
> > +      return const_cast<unsigned char*>(reinterpret_cast<const unsigned char*>(ptr));
> > +    }
> > +    explicit operator void*() const {
> 
> These four const char*, const unsigned char*, unsigned char*, void* explicit
> operators are blargh.  How many instances were we actually hitting where
> these were necessary?  If it's relatively "few" I would probably go more for
> uglifying casts at those sites, or in some utility method in some central
> header that did the relevant sequence of casts instead.  (This would be
> followup-land, to be clear.)

So, how many of these are we hitting on mingw?

Comment 5

[Jacek Caban]

I will try to get those numbers, but this will have to wait at least until tomorrow.

Comment 6

[Jacek Caban]

Attachment: deprecated.diff

I used the attached patch to count use cases of those operators. This makes GCC emit a warning for each case. There are fewer of those than I expected - only 20 for 64-bit debug build. I will attach grepped logs.

Comment 7

[Jacek Caban]

Attachment: deprecated.log

Comment 8

[Jacek Caban]

Note that this will only catch cases like (char*)str. reinterpret_casts will not emit warnings.

Comment 9

[Jacek Caban]

Attachment: Constify aDataBuff in CreatePrimitiveForData.

This was an easy one.

Comment 10

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfeb49413f4e

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bfeb49413f4e