Here's the OIIO side proposed change:
https://github.com/OpenImageIO/oiio/pull/2047
And the cleanup & bug fixes from OSL:
https://github.com/imageworks/OpenShadingLanguage/pull/929
> On Oct 29, 2018, at 12:41 AM, Larry Gritz <[email protected]> wrote:
>
> 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]
>> <mailto:[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] <mailto:[email protected]>
>> http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
>
> --
> Larry Gritz
> [email protected] <mailto:[email protected]>
>
>
>
>
> _______________________________________________
> 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
