I bet Daniel knows why.

I haven't actually tried it yet, but I think it's most likely because we have

    inline bool operator==(const ustring& lhs, const char* rhs)

in ustring.h. Your C++ standard library (i.e. GNU libstdc++) probably doesn't have it, and the string literal therefore goes through

    std::string::string(const string::char_type*)

first. Calling the constructor implicitely _and_ interpreting 0 as a pointer constant would require more than one hop, and therefore doesn't happen.

Any volunteers for actually verifying this hypothesis? ;-)

There is this definition in /usr/lib/gcc/x86_64-pc-linux-gnu/4.3.3/include/g++-v4/bits/basic_string.h:

  template<typename _CharT, typename _Traits, typename _Alloc>
    inline bool
    operator==(const basic_string<_CharT, _Traits, _Alloc>& __lhs,
               const _CharT* __rhs)
    { return __lhs.compare(__rhs) == 0; }

I have verified that this is indeed used for this code:

  std::string s;
  s == "foo";

(In reply to comment #3)
> There is this definition in
> /usr/lib/gcc/x86_64-pc-linux-gnu/4.3.3/include/g++-v4/bits/basic_string.h:
> 
>   template<typename _CharT, typename _Traits, typename _Alloc>
>     inline bool
>     operator==(const basic_string<_CharT, _Traits, _Alloc>& __lhs,
>                const _CharT* __rhs)
>     { return __lhs.compare(__rhs) == 0; }

OK. I think this is different because it is a template. See Stroustrup, chapter 13.3.2:  Templates are resolved _before_ overloads or extra type conversions are considered. At that point, the compiler looks for a match to the argument list

    (const std::string&, int)

and the operator==() template as defined above won't match that, and that's it.

std::min() and std::max(), for instance, also exhibit this behavior -- both arguments have to be of the same type if the template type is to be derived implicitly.

So I guess that means there is not much we can do about this, right?

(In reply to comment #5)
> So I guess that means there is not much we can do about this, right?

Well, we could remove the shortcut overloads for 'const char*' arguments and rely on the implicit conversion from 'const char*' to Glib::ustring.

However, while this would work, I'm not sure it's actually worth it. Actually, I don't think the problem you hit even qualifies as a bug. You could still pass a nil pointer indirectly. It's just a quirk of the C++ language that it doesn't have a context-independent literal for the nil pointer.

So, personally, I'd prefer to close this as either WONTFIX or NOTABUG. Any objections?

Can we maybe add a g_assert or a g_return_val_if_fail to the comparison operator if the const char* parameter is 0? There is already a warning from g_utf8_collate in that case, but this would make it easier to find out where the warning comes from. It would probably have saved me an hour.

Otherwise, I agree that it's not worth removing the const char* overload, also considering that it's probably inefficient to creating a temporary ustring just for comparison.

Definitely not to operator==(), because that's an inline function living in the header file. g_return_if_fail(), g_assert(), g_warning() and the like should never be used in a header file, since the string literals passed to it would no longer live in the library but pollute every single object file which makes use of the inline function in any way.

It could be added to Glib::ustring::compare(const char* rhs), however. But would that actually have helped? I like using g_return_if_fail(), but in this case it seems a bit arbitrary to put it here but not anywhere else. Generally we don't duplicate assertions of the underlying C functions, as a back trace in gdb would immediately reveal the caller anyway.

Would you have been any faster to find the source of the problem if the assertion had been in ustring::compare() instead of g_utf8_collate()?

I'm not totally opposed to the idea of adding a g_return_if_fail(), but I'm not convinced yet that there is much of a point in doing so.

(In reply to comment #8)
> Would you have been any faster to find the source of the problem if the
> assertion had been in ustring::compare() instead of g_utf8_collate()?

I basically ignored the warning from g_utf8_collate since I was looking for a network problem, not for a string issue. Until I realized that I thought the string variable which I compared to 0 was an integer.

It would have let me know that the problem is somewhere in the C++ part of the code. It might have helped, but I'm not sure. Maybe not.

> I'm not totally opposed to the idea of adding a g_return_if_fail(), but I'm not
> convinced yet that there is much of a point in doing so.

Yeah, maybe let's wait. We can still add it later when others hit the same problem (and believe it helped).

Created attachment 317124 [details]
Test case

Some "delete" declarations can fix the problem. The test case contains

  bool operator==(const ustring& lhs, int rhs) = delete;
  bool operator==(int lhs, const ustring& rhs) = delete;

We can add such declaractions to ustring.h. Also for operator!=().
"if (string1 == 0)" would generate a compile-time message like

  error: use of deleted function 

while "if (string1 == nullptr)" would pass the compilation and generate a
runtime message as before.

Without these declarations the comparisons 'string1 == 1' and '1 == string2'
behave quite unexpectedly. They call
  bool operator==(const Glib::QueryQuark& a, const Glib::QueryQuark& b)
because QueryQuark has non-explicit constructors that can convert a ustring
and a GQuark (which is guint32) to QueryQuark. No compile-time error message,
no runtime error message! Even worse than 'string1 == 0'!

Shall I add some such "delete" declarations?

Sounds good to me.

https://git.gnome.org/browse/glibmm/commit/?id=38fd0d5f90683075955a36ce7df7a77d689a34e1