Skip to content

Replace std::to_string with operator<<(llvm::raw_ostream&) #208

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

Merged
merged 3 commits into from
Jan 2, 2018

Conversation

goldsborough
Copy link
Contributor

This PR fixes #180 by replacing our overloads of std::to_string, which is UB, with corresponding llvm::raw_ostream& operator<<(llvm::raw_ostream&, <thing>) overloads.

This was less trivial than thought, because we were using a mix of std::ostream, std::stringstream and llvm::raw_ostream in the codebase. Furthermore, raw_ostream cannot output to files, while std::ostream can, so the replacement wasn't a simple find+replace. I think the best way forward is:

  1. To make a class convertible to string, overload the operator<<(raw_ostream&),
  2. For printing to stderr or stdout, use llvm::outs() and llvm::errs() as the stream source,
  3. To construct a (formatted) string, use llvm::raw_string_ostream or llvm::raw_svector_stream and then call its .str() method,
  4. For file output (which raw_ostream can't do), do (3) and to output it to a std::ofstream.

I think this PR also fixes a bug where we were calling std::to_string on a string literal (in examples/ptb.cpp), which was actually creating a string with the textual representation of the literal's memory address (because of the void* overload we had for std::to_string), rather than constructing a string.

I also recommend using one of my favorite LLVM data structures ever, llvm::Twine, for simple string concatenation instead of std::string::operator+.

It's a lot of code, so please do look at it carefully. Also, please tell me what would be a good test to make sure the dotty output for the IR looks good (@artemrakhov)?

@nadavrot
Copy link
Contributor

nadavrot commented Jan 2, 2018

The changes look good to me. Please run M.dumpDAG() and G.dumpDAG() and M.dump() and G.dump() and make sure that things work.

@goldsborough
Copy link
Contributor Author

Tested the graph and IR dump and found a bug which I fixed. Generated dot graphs are now identical again so the patch is safe (hopefully).

@goldsborough goldsborough merged commit 86a4744 into pytorch:master Jan 2, 2018
@goldsborough goldsborough deleted the replace-to-string branch January 2, 2018 21:15
@nadavrot
Copy link
Contributor

nadavrot commented Jan 2, 2018

Thank you!

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

Successfully merging this pull request may close these issues.

Overloading std::to_string is undefined behavior
3 participants