-
Notifications
You must be signed in to change notification settings - Fork 17
JsonLogTask: fix/improve value comparisons #45
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
JsonLogTask: fix/improve value comparisons #45
Conversation
83e7df4 to
324c2c2
Compare
caPutLogApp/caPutJsonLogTask.cpp
Outdated
| // Doesn't mask "hidden" bytes (after NUL) for the sake of efficiency | ||
| // For string scalars, size == 1 |
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.
To be clear, this comment is just pointing out that this will still return some false negetives and print a log, depending on the junk bytes behind the null. So this is an improvement but not perfect?
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.
Correct.
Doing min(strlen(), MAX_LENGTH) for every element seemed excessive.
caPutLogApp/caPutJsonLogTask.cpp
Outdated
| SINGLE_TYPE_COMPARE(string, MAX_STRING_SIZE); | ||
| // Doesn't mask "hidden" bytes (after NUL) for the sake of efficiency | ||
| // For string scalars, size == 1 | ||
| return memcmp(pa->v_string, pb->v_string, size*MAX_STRING_SIZE) == 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.
Is there some guarantee that bytes following a terminating nil will also be nil? eg. does "foo\0blah" equal "foo\0\0\0\0\0"? memcmp() will say "no", strncmp() will say "yes".
Also, the preceding macro and switch statement could be mostly replaced by use of dbValueSize().
Something like:
size_t size = pLogData->is_array ? pLogData->old_log_size : 1;
if(pLogData->type==DBR_STRING) {
for(size_t i=0; i<size; i++) {
if(strncmp(pa->a_string[i], pb->a_string[i], MAX_STRING_SIZE)!=0) {
return 1;
}
}
} else {
return memcmp(pa, pb, size*dbValueSize(pLogData->type))!=0;
}
return 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.
Is there some guarantee that bytes following a terminating nil will also be nil? eg. does
"foo\0blah"equal"foo\0\0\0\0\0"?memcmp()will say "no",strncmp()will say "yes".
No, there isn't. (See above.)
I found doing multiple memcmp/strncmp excessive, maybe it's not.
Given that arrays of strings are pretty rare...
Also, the preceding macro and switch statement could be mostly replaced by use of
dbValueSize().Something like:
size_t size = pLogData->is_array ? pLogData->old_log_size : 1; if(pLogData->type==DBR_STRING) { for(size_t i=0; i<size; i++) { if(strncmp(pa->a_string[i], pb->a_string[i], MAX_STRING_SIZE)!=0)) { return 1; } } } else { return memcmp(pa, pb, size*dbValueSize(pLogData->type))!=0; } return 0;
That's clearly beyond the original scope of this fix, but very reasonable.
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.
No, there isn't. (See above.)
Ah, now that exchange registers with me.
imo. there seems almost no point to memcmp() past the first terminating nil. The chances of a successful equality seem vanishing small (return 1.
- fix string comparison (was comparing pointers) (fixes epics-modules#43) - improve value comparison code - REF improve name of compare function
324c2c2 to
25b50aa
Compare
anjohnson
left a comment
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.
Much simpler, LGTM!
|
Superseded by #50 |
fixes #43