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

What exactly "write_to_string" expected to do? #10

Open
r7vme opened this issue Oct 31, 2021 · 3 comments
Open

What exactly "write_to_string" expected to do? #10

r7vme opened this issue Oct 31, 2021 · 3 comments

Comments

@r7vme
Copy link

r7vme commented Oct 31, 2021

Hello, I'm trying to understand what write_to_string expected to do. For example, for rosidl_runtime_c__U16String with content "Hello World", I expect that resulted std::string will also have contents "Hello World". But because write_to_string bruteforce fully copies underlay memory, I see that after every char there is a null byte (which is treated as string terminate character).

When I try to print such string I get smth like

H       
e       
l       
l       
o       
        
W       
o       
r       
l       
d 
    

String conversion between std::string and std::wstring is quite complex topic, coversion may not succeed (narrowing conversion from char16 to char8). I'm not generally sure that what you do here is right, because as a result you get ill-formed string.

Should instead smth like std::codecvt_utf8_utf16 be used or wstring considered unsupported by rosidl_typesupport_protobuf?


More details

underlay memory

48 0 65 0 6c 0 6c 0 6f 0 20 0 57 0 6f 0 72 0 6c 0 64 0 <-- initial rosidl_runtime_c__U16String
48 0 65 0 6c 0 6c 0 6f 0 20 0 57 0 6f 0 72 0 6c 0 64 0 <-- converted std::string
48 65 6c 6c 6f 20 57 6f 72 6c 64 <-- expected std::string

following code was used to print above

void print_memory( const void * mem, unsigned int n ) {                                                                                                                                                                               
  const char * p = reinterpret_cast< const char *>( mem );                                                                                                                                                                            
  for ( unsigned int i = 0; i < n; i++ ) {                                                                                                                                                                                            
     std::cout << std::hex << int(p[i]) << " ";                                                                                                                                                                                       
  }                                                                                                                                                                                                                                     std::cout << std::endl;                                                                                                                                                                                                             
}                                                                                                                                                                                                                                     
                                                                                                                                                                                                                                      
TEST(test_wstring_conversion, write_to_string) {                                                                                                                                                                                      
  rosidl_runtime_c__U16String wstr;                           
  ASSERT_TRUE(rosidl_runtime_c__U16String__init(&wstr));                                                                                   
  ASSERT_TRUE(rosidl_runtime_c__U16String__assign(&wstr, reinterpret_cast<const uint16_t*>(u"Hello World")));                                                                                                                         
  std::string converted_str;                                                                                                                                                                                                          
  rosidl_typesupport_protobuf_c::write_to_string(wstr, converted_str);                                                                                                                                                                
  std::string expected_str{"Hello World"};                                                                                                                                                                                            
  print_memory(wstr.data, wstr.size * (sizeof(uint16_t) / sizeof(char)));                                                                                                                                                             
  print_memory(converted_str.data(), converted_str.size());                                                                                                                                                                           
  print_memory(expected_str.data(), expected_str.size());                                                                                                                                                                             
}  
@brakmic-aleksandar
Copy link
Contributor

Since protobuf doesn't support wstrings so we are using bytes field to transfer wstring data, bytes field is reperesented as std::string in generated code, this function only copies wstring as is to bytes field.

@r7vme
Copy link
Author

r7vme commented Nov 4, 2021

we are using bytes field to transfer wstring data

@brakmic-aleksandar got it, I think this was my main confusion. Then may be to avoid confusion for others function signature can be smth like

void write_to_byte_array(const rosidl_runtime_c__U16String &u16str, std::vector<std::byte> &byte_array)

I can submit PR later.

@brakmic-aleksandar
Copy link
Contributor

brakmic-aleksandar commented Nov 4, 2021

Yeah, that's a good idea, thanks!
Second parameter should still stay std::string, because "bytes" field is of that type in C++ generated code.

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

No branches or pull requests

2 participants