Add char16_t* overload of nsString::StripChars

RESOLVED FIXED in Firefox 53

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: emk, Unassigned)

Tracking

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
Because otherwise nsString (and subclasses) will fail to call the |nsAString::StripChars(const char16_t*...| overload.
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8812451 - Flags: review?(nfroyd)

Comment 2

a year ago
mozreview-review
Comment on attachment 8812451 [details]
Bug 1318857 - Unhide nsTSubstring_CharT::StripChars in nsTString_CharT.

https://reviewboard.mozilla.org/r/94190/#review94396

I believe you when you say this is needed, but I'm blanking on why:

http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.h#842

doesn't work here, since `char_type` for `nsAString` there is `char16_t`.  So these methods have identical signatures, right?  Can you elaborate on that?  `nsTString` is publically inheriting from `nsTSubstring`, so the above method ought to be visible on `nsTString`...but it's not, for some reason?

r=me assuming that the answer to the above is reasonable.
Attachment #8812451 - Flags: review?(nfroyd) → review+
(Reporter)

Comment 3

a year ago
(In reply to Nathan Froyd [:froydnj] from comment #2)
> I believe you when you say this is needed, but I'm blanking on why:
> 
> http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.h#842
> 
> doesn't work here, since `char_type` for `nsAString` there is `char16_t`. 
> So these methods have identical signatures, right?  Can you elaborate on
> that?  `nsTString` is publically inheriting from `nsTSubstring`, so the
> above method ought to be visible on `nsTString`...but it's not, for some
> reason?

Honestly, I also have no idea why

>    nsAdoptingString blacklist;
>    nsresult rv = mozilla::Preferences::GetString("network.IDN.blacklist_chars",
> (snip)
>      nsAString& chars = blacklist;
>      chars.StripChars(u" \u3000");

works, but 

>    nsAdoptingString blacklist;
>    nsresult rv = mozilla::Preferences::GetString("network.IDN.blacklist_chars",
> (snip)
>      blacklist.StripChars(u" \u3000");

does not. Do you know the reason why? Or do you have a better idea to satisfy compilers?

I encountered the problem in bug 1143644.
Flags: needinfo?(nfroyd)
(In reply to Masatoshi Kimura [:emk] from comment #3)
> (In reply to Nathan Froyd [:froydnj] from comment #2)
> > I believe you when you say this is needed, but I'm blanking on why:
> > 
> > http://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.h#842
> > 
> > doesn't work here, since `char_type` for `nsAString` there is `char16_t`. 
> > So these methods have identical signatures, right?  Can you elaborate on
> > that?  `nsTString` is publically inheriting from `nsTSubstring`, so the
> > above method ought to be visible on `nsTString`...but it's not, for some
> > reason?
> 
> Honestly, I also have no idea why
> 
> >    nsAdoptingString blacklist;
> >    nsresult rv = mozilla::Preferences::GetString("network.IDN.blacklist_chars",
> > (snip)
> >      nsAString& chars = blacklist;
> >      chars.StripChars(u" \u3000");
> 
> works, but 
> 
> >    nsAdoptingString blacklist;
> >    nsresult rv = mozilla::Preferences::GetString("network.IDN.blacklist_chars",
> > (snip)
> >      blacklist.StripChars(u" \u3000");
> 
> does not. Do you know the reason why? Or do you have a better idea to
> satisfy compilers?

Hm, that's strange!  nsAdoptingString eventually winds up inheriting from nsTSubstring as well, so the method *ought* to be visible...what does the compiler error for the latter case say?

This is all in libxul, so there shouldn't be any internal/external string API issues...
Flags: needinfo?(nfroyd) → needinfo?(VYV03354)
(Reporter)

Comment 5

a year ago
GCC 5.1: https://ideone.com/faTnyx
> prog.cpp: In function 'int main()':
> prog.cpp:15:18: error: no matching function for call to 'nsString::StripChars(const char16_t [1])'
>   s.StripChars(u"");
>                   ^
> prog.cpp:10:7: note: candidate: void nsString::StripChars(const char*)
>   void StripChars(const char* aSet) {}
>        ^
> prog.cpp:10:7: note:   no known conversion for argument 1 from 'const char16_t [1]' to 'const char*'

I got a similar error with MSVC 2015, so it would not be a compiler bug. We need a help from a C++-guru...
Flags: needinfo?(VYV03354)
(Reporter)

Comment 6

a year ago
This compiles: https://ideone.com/KjCqoy
This also compiles: https://ideone.com/GHc7e3
Oh, oh, I see why this is.  nsTSubstring has StripChars methods, but then nsTString has a StripChars method, and the compiler won't do overload resolution with the subclass's methods.  Sigh.

Patch is fine, thank you for investigating!

Comment 8

a year ago
mozreview-review
Comment on attachment 8812451 [details]
Bug 1318857 - Unhide nsTSubstring_CharT::StripChars in nsTString_CharT.

https://reviewboard.mozilla.org/r/94190/#review94406

::: xpcom/string/nsTString.h:373
(Diff revision 1)
> +#ifdef CharT_is_PRUnichar
> +  void StripChars(const char16_t* aChars, uint32_t aOffset = 0)
> +  {
> +    nsTSubstring_CharT::StripChars(aChars, aOffset);
> +  }
> +#endif

Actually, having discovered the nature of the problem, we can just say:

`  using nsTSubstring_CharT::StripChars;`

here instead of the `#ifdef` business, and all should be well.  Can you do that patch instead, please?
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8812451 - Flags: review+ → review?(nfroyd)

Comment 10

a year ago
mozreview-review
Comment on attachment 8812451 [details]
Bug 1318857 - Unhide nsTSubstring_CharT::StripChars in nsTString_CharT.

https://reviewboard.mozilla.org/r/94190/#review94572

Thank you!
Attachment #8812451 - Flags: review?(nfroyd) → review+

Comment 11

a year ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/89665a27aaab
Unhide nsTSubstring_CharT::StripChars in nsTString_CharT. r=froydnj

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/89665a27aaab
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.