[PATCH] D30266: Move Stream code from MSF -> Support

[Zachary Turner via Phabricator via llvm-commits]

zturner added inline comments.


================
Comment at: llvm/lib/Support/BinaryStreamWriter.cpp:28
+
+Error BinaryStreamWriter::writeZeroString(StringRef Str) {
+  if (auto EC = writeFixedString(Str))
----------------
echristo wrote:
> This seems like you want to say "writeTerminatedString"?
> 
> I'd probably go with "writeCString" or "writeString" and use "writeUnterminatedString"? Or something?
Yea, I could call it something else.  But this function is a little weird in that the input itself need not be null terminated.  So even if `Str` is not null terminated, it will write the null terminator anyway.  I'm not sure if changing the name to `writeCString` would make it confusing.  And `writeNullTerminatedString` would be too long.  So `writeZeroString` or `writeTerminatedString` seems the best, but I'm not tied to either one (Zero just seems the least verbose and also specifies what the terminator is, rather than it being some arbitrary character)


================
Comment at: llvm/lib/Support/BinaryStreamWriter.cpp:42
+
+  Offset += Str.size();
+  return Error::success();
----------------
echristo wrote:
> Don't we already update the offset in writeBytes?
If you were to call `this->writeBytes` then yes it would update the offset.  This writes them directly to the underlying stream (which doesn't maintain an offset, only the writer maintains the offset).

But this raises the question of why don't I just call `this->writeBytes` directly.  Good question!  I'll fix that.


https://reviews.llvm.org/D30266