If you have the following code: > nsAutoCString a; > nsAutoString b; > GetSomeString(b); > a.Append(b); It would successfully compile (at least on MSVC), but the result would be wrong (a would get the reinterpreted UTF-16 rather than a converted UTF-8 string). From debugger, it seems that this is because nsTSubstring<char16_t> somehow has operator mozilla::Span<uint8_t>, and nsTSubstring<char> has an Append overload which accepts mozilla::Span<uint8_t>.

> it seems that this is because nsTSubstring<char16_t> somehow has operator mozilla::Span<uint8_t> That's indeed bad and unexpected. It should only have operator mozilla::Span<char16_t>. cpeterson, do you happen to have a fix for this already?

I have a wip patch to prevent implicit construction of Span<char>. I'll post that soon. I haven't looked at this particular case of nsAutoStrings, but I'll check.

IIUC the nsTSubstring conditions <typename EnableIfChar = IsChar> and <typename EnableIfChar16 = IsChar16> should be mutually exclusive, but *both* conditions are true for both nsString (char16_t) and nsCString (char) strings. Unfortunately, this EnableIf template magic is beyond me, so I won't be much help here. This template issue is different from the implicit Span<const char> constructor issue I have a patch for.

Eric, your templatized string classes from bug 1393230 seem to be enabling both <typename EnableIfChar = IsChar> and <typename EnableIfChar16 = IsChar16> for both nsString (char16_t) and nsCString (char) strings.

(In reply to Chris Peterson [:cpeterson] from comment #4) > Eric, your templatized string classes from bug 1393230 seem to be enabling > both <typename EnableIfChar = IsChar> and <typename EnableIfChar16 = > IsChar16> for both nsString (char16_t) and nsCString (char) strings. There were a lot of changes in that bug, can you link specifically to the functions that are having issues?

The specific function in this bug is operator mozilla::Span<uint8_t>(), but I think cpeterson figured out that it is a more general issue that <typename EnableIfChar = IsChar> and <typename EnableIfChar16 = IsChar16> conditions are not effective anymore, so functions and operators are unexpectedly enabled on different string types.

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #6) > The specific function in this bug is operator mozilla::Span<uint8_t>(), but > I think cpeterson figured out that it is a more general issue that <typename > EnableIfChar = IsChar> and <typename EnableIfChar16 = IsChar16> conditions > are not effective anymore, so functions and operators are unexpectedly > enabled on different string types. Oh that's very bad indeed. I'll take a look.

For example, adding the following member functions to nsTSubstring produces a "class member cannot be redeclared" compilation error: template <typename EnableIfChar = IsChar> void ThereCanBeOnlyOne() {} template <typename EnableIfChar16 = IsChar16> void ThereCanBeOnlyOne() {}

In order to properly disable template functions with `std::enable_if` we need to use the resulting type. This only works if we use a dependent type in the template params, hence the need to shadow the `T` param. Proper usage is now of the form: template<typename Q = T, typename EnableIfChar = CharOnlyT<Q>> Foo(); Nathan can you take a look? I know this is kind of awful but AFAICT this is the least-awful way to disable these functions. I'm open changing the naming etc.

Comment on attachment 8916765 [details] [diff] [review] Properly disable string functions for invalid char types Review of attachment 8916765 [details] [diff] [review]: ----------------------------------------------------------------- This is sooo ugly, but I don't think there's a better way to accomplish this task. ::: xpcom/string/nsTDependentString.h @@ +72,1 @@ > nsTDependentString(char16ptr_t aData, uint32_t aLength) One option for making this *slightly* nicer might be: template <typename Q = T> nsTDependentString(IsChar16T<Q, char16ptr_t> aData, uint32_t aLength) with IsChar16T defined something like: template<typename T, typename U> using IsChar16T = typename std::enable_if<std::is_same<char16_t, T>::value, U>::type That would also avoid the slight confusion with using the single-argument form of enable_if, below. ::: xpcom/string/nsTString.h @@ +204,2 @@ > int32_t Find(const char_type* aString, int32_t aOffset = 0, > int32_t aCount = -1) const; These two seem weird to me, why are they templated as being 16-bit only? Oh, because we have a Find(const char*, ...) overload somewhere, and we are trying to declare a const char_type* overload, but only for 16-bit so as to not hit duplicate method declarations? Maybe we should just merge the two implementations so we can have a single const char_type* declaration and implementation? @@ +238,2 @@ > int32_t RFind(const char_type* aString, int32_t aOffset = -1, > int32_t aCount = -1) const; I assume that's what we're doing here and elsewhere. ::: xpcom/string/nsTStringRepr.h @@ +67,5 @@ > +// Please note that we had to use a separate type `Q` for this to work. You > +// will get a semi-decent compiler error if you use `T` directly. > + > +template <typename CharType> using CharOnlyT = > + typename std::enable_if<std::is_same<char, CharType>::value>::type; This appears to be missing the second argument to enable_if? Or we not care about this, because we're only ever using CharOnlyT as a "enable this" and never for its actual result type? @@ +70,5 @@ > +template <typename CharType> using CharOnlyT = > + typename std::enable_if<std::is_same<char, CharType>::value>::type; > + > +template <typename CharType> using Char16OnlyT = > + typename std::enable_if<std::is_same<char16_t, CharType>::value>::type; This one too.

(In reply to Nathan Froyd [:froydnj] from comment #10) > Comment on attachment 8916765 [details] [diff] [review] > Properly disable string functions for invalid char types > > Review of attachment 8916765 [details] [diff] [review]: > ----------------------------------------------------------------- > > This is sooo ugly, but I don't think there's a better way to accomplish this > task. > > ::: xpcom/string/nsTDependentString.h > @@ +72,1 @@ > > nsTDependentString(char16ptr_t aData, uint32_t aLength) > > One option for making this *slightly* nicer might be: > > template <typename Q = T> > nsTDependentString(IsChar16T<Q, char16ptr_t> aData, uint32_t aLength) > > with IsChar16T defined something like: > > template<typename T, typename U> > using IsChar16T = typename std::enable_if<std::is_same<char16_t, T>::value, > U>::type > > That would also avoid the slight confusion with using the single-argument > form of enable_if, below. I kind of prefer keeping the function params less ugly. dmajor has chimed in on the aesthetics before, lets see what he thinks. > ::: xpcom/string/nsTString.h > @@ +204,2 @@ > > int32_t Find(const char_type* aString, int32_t aOffset = 0, > > int32_t aCount = -1) const; > > These two seem weird to me, why are they templated as being 16-bit only? > Oh, because we have a Find(const char*, ...) overload somewhere, and we are > trying to declare a const char_type* overload, but only for 16-bit so as to > not hit duplicate method declarations? Maybe we should just merge the two > implementations so we can have a single const char_type* declaration and > implementation? Yeah these are weird, what's really going on is that we don't support a case insensitive comparison for `char16_t` *unless* comparing against a `char`. In general I think we should try to get rid of these monstrosities. See bug 1397045. > @@ +238,2 @@ > > int32_t RFind(const char_type* aString, int32_t aOffset = -1, > > int32_t aCount = -1) const; > > I assume that's what we're doing here and elsewhere. Yeah same deal. > ::: xpcom/string/nsTStringRepr.h > @@ +67,5 @@ > > +// Please note that we had to use a separate type `Q` for this to work. You > > +// will get a semi-decent compiler error if you use `T` directly. > > + > > +template <typename CharType> using CharOnlyT = > > + typename std::enable_if<std::is_same<char, CharType>::value>::type; > > This appears to be missing the second argument to enable_if? Or we not care > about this, because we're only ever using CharOnlyT as a "enable this" and > never for its actual result type? Correct, it just defaults to `void` but we don't care about it.

Comment on attachment 8916765 [details] [diff] [review] Properly disable string functions for invalid char types Review of attachment 8916765 [details] [diff] [review]: ----------------------------------------------------------------- David, we had to add back in the slightly awful `template<typename Q = T, typename EnableIfChar = CharOnlyT<Q>>` format to the `nsString` functions. I'd like to get your feedback on the readability (I know it's not great, but I think it's as good as it gets). Nathan has an alternate suggestion in comment 10, do you have a preference between the two or a suggested improvement?

(In reply to Nathan Froyd [:froydnj] from comment #10) > One option for making this *slightly* nicer might be: > > template <typename Q = T> > nsTDependentString(IsChar16T<Q, char16ptr_t> aData, uint32_t aLength) I think that makes my head hurt even more. :-) I'm okay-ish with the current patch, though the "Q" still screams to me "there is a thing here that I don't understand, I need to stop and look at the header to figure out what this is supposed to mean." Maybe I'd have an easier time skimming past it if the name suggested "nothing to see here"? Maybe s/Q/Dummy/?

Oh wait, I didn't look closely enough at what you're doing. "Dummy" would look pretty bad in the headers. Maybe T_?

(In reply to David Major [:dmajor] from comment #14) > Oh wait, I didn't look closely enough at what you're doing. "Dummy" would > look pretty bad in the headers. Maybe T_? Sounds good, that seems a bit better than a random 'Q'.

(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #15) > (In reply to David Major [:dmajor] from comment #14) > > Oh wait, I didn't look closely enough at what you're doing. "Dummy" would > > look pretty bad in the headers. Maybe T_? > > Sounds good, that seems a bit better than a random 'Q'. I dunno, T, U, and maybe V are pretty standard names for generic template parameters. Q doesn't seem any worse than T_, and is more visually distinct from `T`.

But the idea is that this thing _is_ T, right?

This adds a simple test that makes sure the enable_if stuff is working.

(In reply to David Major [:dmajor] from comment #17) > But the idea is that this thing _is_ T, right? I somewhat agree with Nathan so I'm just keeping it 'Q'. I've added attached a patch to test that this stuff at least works.

Just waiting on a green windows build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9218444c4e2d895ed3950246c26c4552acc00b60

Pushed by erahm@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0d2b1057b968 Part 1: Properly disable string functions for invalid char types. r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/b871e93e7912 Part 2: Test conditionaly enabling string functions. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/0d2b1057b968 https://hg.mozilla.org/mozilla-central/rev/b871e93e7912