Date
1 - 6 of 6
Review: string routine drives me crazy (issue194067)
cku...@...
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
LGTM, but this is quite mysterious. Were the crashes on OS X or linux?
http://codereview.appspot.com/194067/show
Brian Budge <brian...@...>
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
toggle quoted message
Show quoted text
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, <larry...@...> 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...@....
To unsubscribe from this group, send email to
osl...@....
For more options, visit this group at
http://groups.google.com/group/osl-dev?hl=en.
Larry Gritz <l...@...>
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:
Larry Gritz
l...@...
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, <larry...@...> 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...@....
To unsubscribe from this group, send email to
osl...@....
For more options, visit this group at
http://groups.google.com/group/osl-dev?hl=en.
Larry Gritz
l...@...
Larry Gritz <l...@...>
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:
Larry Gritz
l...@...
-- 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
l...@...
Brian Budge <brian...@...>
Nice. You're correct, there's no robustness issue, I just had a brain-blip.
toggle quoted message
Show quoted text
On Mon, Jan 25, 2010 at 4:06 PM, Larry Gritz <l...@...> 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
l...@...
--
You received this message because you are subscribed to the Google Groups "OSL Developers" group.
To post to this group, send email to osl...@....
To unsubscribe from this group, send email to osl...@....
For more options, visit this group at http://groups.google.com/group/osl-dev?hl=en.
Larry Gritz <l...@...>
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, <cku...@...> wrote:
Larry Gritz
l...@...
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, <cku...@...> 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
l...@...