Code review - Issue 194067: Review: string routine drives me crazyhttps://codereview.appspot.com/2010-01-26T00:09:40+00:00rietveld
Message from unknown
2010-01-25T23:34:09+00:00larrygritzurn:md5:7120eb26d4a6c2c7b0ed198fb3af54a2
Message from larrygritz@gmail.com
2010-01-25T23:34:09+00:00larrygritzurn:md5:fa50a907c13b1eb5d758caa959d0747e
Message from ckulla@gmail.com
2010-01-25T23:38:06+00:00ckullaurn:md5:a56c3a25a85d648685516264de90ae82
On 2010/01/25 23:34:09, larrygritz wrote:
>
LGTM, but this is quite mysterious. Were the crashes on OS X or linux?
Message from brian.budge@gmail.com
2010-01-25T23:51:23+00:00brian.budgeurn:md5:59f7bf66a1e9e509fed98fa7e327f9ff
Hi Larry -
Just to be robust, would it make sense to make sure that source is at
least as long as pattern? Also, is this the correct way to respond to
a code review (in email)?
Brian
On Mon, Jan 25, 2010 at 3:34 PM, <larrygritz@gmail.com> wrote:
> Reviewers: ,
>
> Description:
> Many times I've been plagued by inexplicable crashes in loadshader.cpp,
> right at the call to boost::starts_with. I've wasted hours and hours
> and been unable to figure out why it boost::startswith(x,y) should fail
> when x and y are valid std::strings. So I finally wrote my own
> (trivially) just to see if that happens, and I no longer crash. (Making
> me able to move on to the real crash I'm supposed to be debugging. :-)
>
>
> Please review this at http://codereview.appspot.com/194067/show
>
> Affected files:
> src/liboslexec/loadshader.cpp
>
>
> Index: src/liboslexec/loadshader.cpp
> ===================================================================
> --- src/liboslexec/loadshader.cpp (revision 544)
> +++ src/liboslexec/loadshader.cpp (working copy)
> @@ -232,13 +232,21 @@
>
>
>
> +inline bool
> +starts_with (const std::string &source, const std::string &pattern)
> +{
> + return ! strncmp (source.c_str(), pattern.c_str(), pattern.length());
> +}
> +
> +
> +
> // If the string 'source' begins with 'pattern', erase the pattern from
> // the start of source and return true. Otherwise, do not alter source
> // and return false.
> inline bool
> extract_prefix (std::string &source, const std::string &pattern)
> {
> - if (boost::starts_with (source, pattern)) {
> + if (starts_with (source, pattern)) {
> source.erase (0, pattern.length());
> return true;
> }
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "OSL Developers" group.
> To post to this group, send email to osl-dev@googlegroups.com.
> To unsubscribe from this group, send email to
> osl-dev+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/osl-dev?hl=en.
>
>
Message from lg@imageworks.com
2010-01-26T00:04:57+00:00lg_imageworks.comurn:md5:6362c1b4953b39d47b58a7c3de925fe2
Either email or by going to the link below and commenting using the review tool. Frankly, I prefer email except if you need to use the ability to comment on particular lines in context. Though I can see how others might prefer that ALL review correspondence go through the tool, to keep the conversations together.
As for robustness, why wouldn't strncmp be perfectly robust as I've used it? What do you imagine would go wrong if source was shorter than pattern, other than wasting a few cycles in strncmp?
-- lg
On Jan 25, 2010, at 3:51 PM, Brian Budge wrote:
> Hi Larry -
>
> Just to be robust, would it make sense to make sure that source is at
> least as long as pattern? Also, is this the correct way to respond to
> a code review (in email)?
>
> Brian
>
> On Mon, Jan 25, 2010 at 3:34 PM, <larrygritz@gmail.com> wrote:
>> Reviewers: ,
>>
>> Description:
>> Many times I've been plagued by inexplicable crashes in loadshader.cpp,
>> right at the call to boost::starts_with. I've wasted hours and hours
>> and been unable to figure out why it boost::startswith(x,y) should fail
>> when x and y are valid std::strings. So I finally wrote my own
>> (trivially) just to see if that happens, and I no longer crash. (Making
>> me able to move on to the real crash I'm supposed to be debugging. :-)
>>
>>
>> Please review this at http://codereview.appspot.com/194067/show
>>
>> Affected files:
>> src/liboslexec/loadshader.cpp
>>
>>
>> Index: src/liboslexec/loadshader.cpp
>> ===================================================================
>> --- src/liboslexec/loadshader.cpp (revision 544)
>> +++ src/liboslexec/loadshader.cpp (working copy)
>> @@ -232,13 +232,21 @@
>>
>>
>>
>> +inline bool
>> +starts_with (const std::string &source, const std::string &pattern)
>> +{
>> + return ! strncmp (source.c_str(), pattern.c_str(), pattern.length());
>> +}
>> +
>> +
>> +
>> // If the string 'source' begins with 'pattern', erase the pattern from
>> // the start of source and return true. Otherwise, do not alter source
>> // and return false.
>> inline bool
>> extract_prefix (std::string &source, const std::string &pattern)
>> {
>> - if (boost::starts_with (source, pattern)) {
>> + if (starts_with (source, pattern)) {
>> source.erase (0, pattern.length());
>> return true;
>> }
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "OSL Developers" group.
>> To post to this group, send email to osl-dev@googlegroups.com.
>> To unsubscribe from this group, send email to
>> osl-dev+unsubscribe@googlegroups.com.
>> For more options, visit this group at
>> http://groups.google.com/group/osl-dev?hl=en.
>>
>>
>
--
Larry Gritz
lg@imageworks.com
Message from lg@imageworks.com
2010-01-26T00:06:32+00:00lg_imageworks.comurn:md5:b9c0b864e23da4fbf4e1659551897ab2
Wow, what do you know, apparently if you reply via email and don't change any of the CC's or subject line, the email correspondence DOES get appended to the review trail in the codereview tool! So reply whichever way is more convenient for you.
-- lg
On Jan 25, 2010, at 3:51 PM, Brian Budge wrote:
> Hi Larry -
>
> Just to be robust, would it make sense to make sure that source is at
> least as long as pattern? Also, is this the correct way to respond to
> a code review (in email)?
>
> Brian
--
Larry Gritz
lg@imageworks.com
Message from brian.budge@gmail.com
2010-01-26T00:08:33+00:00brian.budgeurn:md5:0cdc5f68ef8e0ef03fca05d07c43fe9b
Nice. You're correct, there's no robustness issue, I just had a brain-blip.
On Mon, Jan 25, 2010 at 4:06 PM, Larry Gritz <lg@imageworks.com> wrote:
> Wow, what do you know, apparently if you reply via email and don't change any of the CC's or subject line, the email correspondence DOES get appended to the review trail in the codereview tool! So reply whichever way is more convenient for you.
>
> -- lg
>
>
> On Jan 25, 2010, at 3:51 PM, Brian Budge wrote:
>
>> Hi Larry -
>>
>> Just to be robust, would it make sense to make sure that source is at
>> least as long as pattern? Also, is this the correct way to respond to
>> a code review (in email)?
>>
>> Brian
>
> --
> Larry Gritz
> lg@imageworks.com
>
>
>
>
> --
> You received this message because you are subscribed to the Google Groups "OSL Developers" group.
> To post to this group, send email to osl-dev@googlegroups.com.
> To unsubscribe from this group, send email to osl-dev+unsubscribe@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/osl-dev?hl=en.
>
>
Message from lg@imageworks.com
2010-01-26T00:09:40+00:00lg_imageworks.comurn:md5:05cb4ed9fee6eb1c1fedb781a6235a17
OS X. I haven't seen it on Linux, but I'm not sure that means much.
It's black magic to be sure, but I just can't justify spending any more time tracking down this red herring when I have legit bugs to pursue.
-- lg
On Jan 25, 2010, at 3:38 PM, <ckulla@gmail.com> wrote:
> On 2010/01/25 23:34:09, larrygritz wrote:
>
>
> LGTM, but this is quite mysterious. Were the crashes on OS X or linux?
>
> http://codereview.appspot.com/194067/show
>
--
Larry Gritz
lg@imageworks.com