TommyKaneko / Sketchup-API-C-Wrapper

A complete set of C++ Wrapper classes for SketchUp C API objects
MIT License
28 stars 8 forks source link

String::std_string() returns null terminator as part of string. #37

Closed jimfoltz closed 6 years ago

jimfoltz commented 6 years ago

https://github.com/TommyKaneko/Sketchup-API-C-Wrapper/blob/3f06f164448163b92bb12161bb694115899d056d/src/SUAPI-CppWrapper/String.cpp#L155

a char* char_array needs to be null terminated. A vector<char> does not. No need to add 1 here.

jimfoltz commented 6 years ago

Although maybe it's more correct to "chomp" the returned vector.

TommyKaneko commented 6 years ago

No, as long as it works without the extra character, we can get rid of it. Please test and make a pull request.

thomthom commented 6 years ago

That vector or char is fed to SUStringGetUTF8 which expects char*. Further more the documentation describe:

This function does not allocate memory. You must provide an array of sufficient length to get the entire string. The output string will be NULL terminated.

The +1 to the vector size is to ensure enough memory is allocated.

std::string is then receiving a NULL terminated char* - which is standard way to represent a string in C. It knows to stop at NULL. No need to "chomp".

Unless anyone is seeing incorrect string lengths returned? (That would be a good case for setting up in a test unit.)

jimfoltz commented 6 years ago

I am seeing an extra char - that's why I brought it up.

jimfoltz commented 6 years ago

It may bot be the null terminator, but I do get some extra thing at the end of a string. This image is from windows command line using less.exe pager. So some maybe binary thing there - no time now to investigate.

00513

thomthom commented 6 years ago

hmm...

That would be junk uninitialised data. Might be the creation of the vector;

std::vector<char> char_array(out_length);

Does it change if it's all initialized to null?

std::vector<char> char_array(out_length, 0);

Let me use that as a sample for the unit tests I'm scaffolding.

jimfoltz commented 6 years ago

BTW the GetString() function you postyed has the same issue.

thomthom commented 6 years ago

I'm seeing that the std::string's size is one larger and contain a NULL byte:

image

I'll investigate closer.

thomthom commented 6 years ago

Ah, it was this line:

https://github.com/TommyKaneko/Sketchup-API-C-Wrapper/blob/3f06f164448163b92bb12161bb694115899d056d/src/SUAPI-CppWrapper/String.cpp#L159

That copies all the bytes, including NULL bytes.

Replacing it with std::string str(char_array.data()); made it scan and stop at the NULL terminator.

However, that prevents String from representing binary data.

Maybe it's better to keep using begin() and end() but omit trying to read the extra NULL termination.

thomthom commented 6 years ago

On second though, SUStringSetUTF8 relies on NULL termination, so there is no way it can represent binary data.

I'll switch to .data() and include it in my test unit PR.

thomthom commented 6 years ago

Btw Jim, those weird characters were probably a result of Release mode uninitialized char in the vector. In Debug it looks to init to 0 - but Debug allocate memory differently.