Closed
Bug 1318857
Opened 8 years ago
Closed 8 years ago
Add char16_t* overload of nsString::StripChars
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: emk, Unassigned)
Details
Attachments
(1 file)
Because otherwise nsString (and subclasses) will fail to call the |nsAString::StripChars(const char16_t*...| overload.
Comment hidden (mozreview-request) |
Reporter | ||
Updated•8 years ago
|
Attachment #8812451 -
Flags: review?(nfroyd)
Comment 2•8 years 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•8 years 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)
Comment 4•8 years ago
|
||
(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•8 years 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•8 years ago
|
||
This compiles: https://ideone.com/KjCqoy This also compiles: https://ideone.com/GHc7e3
Comment 7•8 years ago
|
||
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•8 years 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•8 years ago
|
Attachment #8812451 -
Flags: review+ → review?(nfroyd)
Comment 10•8 years 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•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/89665a27aaab
Status: NEW → RESOLVED
Closed: 8 years 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.
Description
•