I'm reviving this thread from almost 4 years ago (!) to apologize to Thiago and
John for not following up.
In order to remove this thread from my festering inbox with a clear conscience,
I did an experiment where I tried removing ustring::operator int() to see if we
rely on it heavily. We only used it in two spots in OIIO itself (easy to change
to ustring.empty()), but when I tried building OSL against it, I discovered TWO
undiscovered bugs in OSL that were the result of the very misunderstanding they
were warning me about.
They were quite right to be skeptical of ustring's operator int()!
So sorry, gentlemen. I should have listened to you earlier.
I've got a fix for OSL coming soon, then will try to figure out how to
deprecate this in OIIO without causing too much pain for anybody who has been
using the "if (myustring)" idiom.
-- lg
> On Jan 26, 2015, at 10:35 AM, Thiago Ize <[email protected]> wrote:
>
> Agreed, it does all seem a bit scary -- just like operator int was scary in
> how it caused unexpected bugs. I didn't mention, but another place operator
> int bit me is in porting over code that made a distinction between a NULL
> pointer and the "\0" string:
>
> void foo(const char* str)
> if (!str)
> printf("NULL string");
> else if(!str[0]) // this is the empty null terminated string
> printf("valid, allocated, empty string");
> else printf("valid non-empty string");
>
> It's completely valid for me to want to differentiate between there being no
> string and the "\0" string. The ustring operator int doesn't make the same
> distinction and so it can make porting code over to ustring difficult since
> everything continues to compile and might look correct, but it can cause
> subtle bugs in code that depended on this distinction. Note that std::string
> doesn't allow this type of usage.
>
> My ideal solution is to just get rid of operator int and force the user to
> make the distinction and write if(!str.empty()), if (str.length()), or in my
> case, if(str.c_str()) instead of if(str). But if we do keep it and at least
> go the std::string conversion route, this, I *think*, will do the trick.
> typedef const std::string& const_string_ref;
> operator const_string_ref() const { return this->string(); }
>
>
> On Mon, Jan 26, 2015 at 10:43 AM, Larry Gritz <[email protected]
> <mailto:[email protected]>> wrote:
> We should note that std::string does NOT have a 'operator char *', and
> perhaps cautiously ponder why that might be. (Neither does std::string have
> 'operator bool', that's probably also worth considering.)
>
> I'm not sure why 'operator char *' makes me more nervous than 'operator
> std::string'.
>
> I also just realized, ustring::str() returns a std::string&, so if you pass
> myustring.str() to something that wants a const std::string&, it won't create
> a new string copy. It's not clear to me yet wether we can make 'operator
> const std::string&', which is really more like what we want. I have a feeling
> there is no way to make that happen.
>
> Hmmm, several things to consider. This is one to consider very carefully
> before adding or subtracting any casts. That feels more dangerous than
> ordinary methods that you'd have to call explicitly.
>
>
>
> On Jan 26, 2015, at 9:11 AM, John Haddon <[email protected]
> <mailto:[email protected]>> wrote:
>
>> As someone who merrily plunged headfirst into this trap a little while back,
>> I thought I might chip in. I don't recall the exact specifics, but I think
>> my problem was triggered when OSLQuery changed a bunch of members from
>> string to ustring a little while back. I had existing code that passed those
>> members to methods expecting std::string, and the code compiled fine with
>> the ustring change, but crashed at runtime. I think the compiler was casting
>> ustring to (invalid) char * via this conversion, and then constructing
>> std::string from invalid memory - does that sound plausible?. It took a
>> while to figure out, because my assumption was "ustring behaves like string
>> as far as possible".
>>
>> In my case I think a specific cast operator to std::string would have kept
>> everything working as intended without change, so that seems like a pretty
>> reasonable approach at least for the specific issue I encountered…
>>
>> Cheers…
>> John
>>
>>
>> From: Oiio-dev [[email protected]
>> <mailto:[email protected]>] on behalf of Thiago Ize
>> [[email protected] <mailto:[email protected]>]
>> Sent: Monday, January 26, 2015 8:40 AM
>> To: OpenImageIO developers
>> Subject: Re: [Oiio-dev] ustring::operator int() is bad idea?
>>
>> An operator std::string() function would be great if you can get that to
>> work! Of course, anything else that was expecting some kind of integer,
>> such as a char or int, will still get caught in this, but until OIIO
>> switches to c++11, I guess this is good enough at getting at least this
>> common case covered.
>>
>> Speaking of operator std::string(), if we're going to do that, would it be a
>> good idea to also add an operator const char*()? Sadly, that won't work
>> with printf since that takes in a vararg, but it would work for regular
>> functions that take in a const char*, such as atoi(). Adding that also has
>> the nice benefit of making the original issue I reported ambiguous.
>>
>> Suppose we have a custom string class called mystring which just like
>> std::string will allow a mystring to be created from either a char or a
>> char*. Then:
>>
>> my_string("string B");
>> my_string = my_ustring;
>>
>> will report:
>> foo.cpp:13:14: error: use of overloaded operator '=' is ambiguous (with
>> operand types 'mystring' and
>> 'OpenImageIO::v1_4::ustring')
>> my_string = my_ustring;
>> ~~~~~~~~~ ^ ~~~~~~~~~~
>> foo.h:507:7: note:
>> candidate function
>> operator=(const _char* __s)
>> ^
>> foo.h:518:7: note:
>> candidate function
>> operator=(char __c)
>> ^
>>
>> This is actually a nice error since it at least prevents this bug from
>> slipping through in non std::string classes and we thereby have all our
>> major use cases covered. Yes, I guess if someone tried to assign a ustring
>> directly to a char then they won't get an error. In that case, maybe
>> rewriting operator int() to be:
>>
>> operator int (void) const {
>> if (!m_chars) return 0;
>> return m_chars[0];
>> }
>>
>> or perhaps better yet, replacing it with a char operator
>>
>> operator char (void) const {
>> if (!m_chars) return 0;
>> return m_chars[0];
>> }
>>
>> would be an even better solution since now we can have all our bases
>> covered. If someone tries to write directly to a char:
>> char c = ustring("x");
>> it will work in a sane truncating kind of way. if(my_ustring) will still
>> work just like before. And if someone writes directly to a const char*, the
>> operator const char* will catch it, as will the operator std::string for
>> std::string. And for anything else that is ambiguous, the compiler will
>> complain.
>>
>> On Sun, Jan 25, 2015 at 11:52 PM, Larry Gritz <[email protected]
>> <mailto:[email protected]>> wrote:
>> I totally see what you're getting at and agree that it's a potential
>> problem. (Though the real source of the problem is that in C++ is that 0
>> casts just tine to a pointer, instead of having a proper pointer type, like
>> nullptr is in C++11. I digress, that observation doesn't actually help us
>> solve this problem in practice.)
>>
>> I'm very afraid of breaking existing code, and also I am still rather fond
>> of the "if (s)" idiom.
>>
>> Might another way of dealing with this be to add
>>
>> operator std::string() const { return this->string(); }
>>
>> That would allow assignment of ustring to std::string to do the obvious
>> thing.
>>
>> Would that suffice, or can you think of other scenarios in which operator
>> bool is likely to be an ongoing problem?
>>
>> -- lg
>>
>>
>>
>> On Jan 25, 2015, at 6:24 PM, Thiago Ize <[email protected]
>> <mailto:[email protected]>> wrote:
>>
>> > I'm wondering if the following ustring function:
>> >
>> > /// Cast to int, which is interpreted as testing whether it's not an
>> > /// empty string. This allows you to write "if (t)" with the same
>> > /// semantics as if it were a char*.
>> > operator int (void) const { return !empty(); }
>> >
>> > is a good idea? Reason being that this allows for what is probably
>> > unexpected behavior in the common use case of trying to assign a ustring
>> > to a std::string (note how this case could also happen from a simple typo
>> > if the "u" in ustring was accidentally omitted). For instance:
>> > ustring my_ustring("string A");
>> > string my_string("string B");
>> > my_string = my_ustring;
>> > printf(" my_string=\"%s\" and\n my_ustring=\"%s\"\n",
>> > my_string.c_str(), my_ustring.c_str());
>> >
>> > will print out:
>> > my_string="" and
>> > my_ustring="string A"
>> >
>> > which is most likely not the intended result. If we get rid of this
>> > function, then the compiler will point out our mistake:
>> > foo.cpp:13:14: error: no viable overloaded '='
>> > my_string = my_ustring;
>> >
>> > I realize getting rid of this function would break lots of preexisting
>> > code, but I think this might be worth it in order to prevent this type of
>> > problem. If it's too big of a change, then at least it might be worth
>> > adding a comment to this function indicating that it will cause this
>> > problem.
>> >
>>
>> --
>> Larry Gritz
>> [email protected] <mailto:[email protected]>
>>
>>
>>
>> _______________________________________________
>> Oiio-dev mailing list
>> [email protected] <mailto:[email protected]>
>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org>
>>
>> _______________________________________________
>> Oiio-dev mailing list
>> [email protected] <mailto:[email protected]>
>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
>> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org>
> --
> Larry Gritz
> [email protected] <mailto:[email protected]>
>
>
>
>
> _______________________________________________
> Oiio-dev mailing list
> [email protected] <mailto:[email protected]>
> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
> <http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org>
>
>
> _______________________________________________
> Oiio-dev mailing list
> [email protected]
> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
--
Larry Gritz
[email protected]
_______________________________________________
Oiio-dev mailing list
[email protected]
http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
