Skip to content
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

DataArray<T>::diff string comparison error #1076

Closed
BradWhitlock opened this issue Feb 7, 2023 · 1 comment · Fixed by #1086
Closed

DataArray<T>::diff string comparison error #1076

BradWhitlock opened this issue Feb 7, 2023 · 1 comment · Fixed by #1086
Labels

Comments

@BradWhitlock
Copy link
Member

The diff method does not appear to compare strings properly if a string has been overwritten into a node that already had a longer string. I assume that it saw that it did not have to reallocate and it stores "quad\0" into the existing buffer. Then diff() subtracts 1 off the end of the char buffer length but that does not reflect the length of the string. So, we end up comparing "quad" vs "quad\0on" and the comparison fails.

  1. Can the code encounter non-null terminated strings? Is that why it's even passing a length to std::string?
  2. If we change to the typical std::string(const char*) constructor (ignoring any lengths) it works - but my strings are null terminated.
node["elements/shape"] = "polygon";
// some other logic
node["elements/shape"] = "quad";

Then I called some code in a test to compare my in-memory value to a baseline read by relay.

conduit::relay::io::load(filename, "yaml", baseline);
bool equal = !baseline.diff(n, info, tolerance, true);
@cyrush
Copy link
Member

cyrush commented Feb 7, 2023

@BradWhitlock
Thanks for the report -- this is a bug, and yes it is a consequence of the logic which avoids reallocation.

I think we can specialize the the char8 DataArray case to do the right thing -- respect NULL termed strings.

Related, the set reallocation avoidance logic causes confusion in many of cases.

I would like to change this -- #941 -- but this would be a breaking change with the potential to cause many surprises.

Long term it makes sense to rip the bandaid off, but we would have to make sure conduit users know this is coming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants