1318857 - Add char16_t* overload of nsString::StripChars

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Masatoshi Kimura [:emk]]

Because otherwise nsString (and subclasses) will fail to call the |nsAString::StripChars(const char16_t*...| overload.

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: Bug 1318857 - Unhide nsTSubstring_CharT::StripChars in nsTString_CharT.

Review commit: https://reviewboard.mozilla.org/r/94190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94190/

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[Masatoshi Kimura [:emk]]

(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.

Comment 4

[Nathan Froyd [:froydnj]]

(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...

Comment 5

[Masatoshi Kimura [:emk]]

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...

Comment 6

[Masatoshi Kimura [:emk]]

This compiles: https://ideone.com/KjCqoy
This also compiles: https://ideone.com/GHc7e3

Comment 7

[Nathan Froyd [:froydnj]]

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

[Nathan Froyd [:froydnj]]

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 9

[Masatoshi Kimura [:emk]]

Comment on attachment 8812451 [details]
Bug 1318857 - Unhide nsTSubstring_CharT::StripChars in nsTString_CharT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94190/diff/1-2/

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8812451 [details]
Bug 1318857 - Unhide nsTSubstring_CharT::StripChars in nsTString_CharT.

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

Thank you!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89665a27aaab
Unhide nsTSubstring_CharT::StripChars in nsTString_CharT. r=froydnj

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89665a27aaab