genotrance / nimfastText

NimfastText is a Nim wrapper for the fastText library
MIT License
4 stars 2 forks source link

toCstring() is strange -- memory error? #6

Open cdunn2001 opened 5 years ago

cdunn2001 commented 5 years ago

(I'm still trying to get nimgen to work on fastText, but as I have it partially working I am now curious about a generated file.) /private/tmp/nimble_66140/githubcom_genotrancenimfastText/nimfastText/src/to_cs tring.cc:

char*  toCstring( const std::string& s ) {
  std::vector<char> v(s.size()+1);
  memcpy( &v.front(), s.c_str(), s.size() + 1 );
  return v.data();
};

Could you explain the memory management there? Why not simply return s.c_str()? Wouldn't v.data() refer to something immediately deleted before this function returns?

cdunn2001 commented 5 years ago

This doesn't work either:

proc constructStdString*(s: cstring): std_string {.importcpp: "std::string(@)", constructor, header: "<string>".}
proc c_str*(s: std_string): cstring {.importcpp: "const_cast<char*>(#.c_str())", header: "<string>".}

proc main =
  echo "hi"
  var s: std_string
  s = constructStdString("foo")
  var q: cstring = s.c_str()
  echo "then:", q
  s = constructStdString("bar")
  echo "now:", q
hi
then:foo
now:

But var q = $s.c_str works great.

genotrance commented 5 years ago

It came from a PR from @bung87: https://github.com/genotrance/nimfastText/pull/4/files.

Perhaps he can explain? If you have a cleaner implementation, I'm happy to take any PRs.

bung87 commented 5 years ago

I not really sure, very newbie to c++, you can make a PR for this.

cdunn2001 commented 5 years ago

Yeah, it's a memory error. I'll try to think of something more convenient than $s.c_str, but at least that's reliable.