-
Notifications
You must be signed in to change notification settings - Fork 301
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HPCC-18173 Couchbase Plugin: Solve various memory leaks #10333
HPCC-18173 Couchbase Plugin: Solve various memory leaks #10333
Conversation
Signed-off-by: Dan S. Camper <dan.camper@lexisnexisrisk.com>
@rpastrana Please review |
https://track.hpccsystems.com/browse/HPCC-18173 |
Automated Smoketest: ✅
Install hpccsystems-platform-community_6.4.1-closedown0.el7.x86_64.rpm Unit tests result:
Regression test result:
HPCC Stop: OK |
m_numParams = countParameterPlaceholders(script); | ||
size32_t len = rtlUtf8Size(chars, script); | ||
|
||
if (len > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly stylistic but could be written as "if (len)"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, but in an alternate universe where len < 0 the subsequent code would then fail. I'm guarding against time-traveling extraterrestrials that use their own operating system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, this assumes rtlUtf8Size doesn't return negatives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcamper looks good. I had a minor comment, but I'm ok leaving the code as is...
@richardkchapman Please merge. |
Signed-off-by: Dan S. Camper dan.camper@lexisnexisrisk.com
Type of change:
Checklist:
Testing:
Tested under Valgrind using a stand-alone application built with eclcc.