Open Bug 943870 Opened 6 years ago Updated 6 years ago

Consider removing char* casts from char16ptr_t.

Categories

(Core :: String, defect)

x86
Windows 7
defect
Not set

Tracking

()

People

(Reporter: jacek, Unassigned)

References

Details

(Whiteboard: leave open)

Attachments

(3 files, 1 obsolete file)

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.)
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
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8341685 - Flags: review?(jwalden+bmo)
This code is not covered by try. It's mingw-specific.
(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!
Status: ASSIGNED → NEW
Assignee: birunthan → nobody
Attachment #8341685 - Attachment is obsolete: true
Attachment #8341685 - Flags: review?(jwalden+bmo)
(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?
I will try to get those numbers, but this will have to wait at least until tomorrow.
Attached patch deprecated.diffSplinter Review
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.
Attached file deprecated.log
Note that this will only catch cases like (char*)str. reinterpret_casts will not emit warnings.
This was an easy one.
Attachment #8345813 - Flags: review?(ehsan)
Attachment #8345813 - Flags: review?(ehsan) → review+
You need to log in before you can comment on or make changes to this bug.