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

[Eric Christopher via Phabricator via llvm-commits]

echristo added inline comments.


================
Comment at: llvm/lib/Support/BinaryStreamWriter.cpp:28
+
+Error BinaryStreamWriter::writeZeroString(StringRef Str) {
+  if (auto EC = writeFixedString(Str))
----------------
zturner wrote:
> 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)
*shrug* If it becomes a problem we can also just rename it :)

Just "writeZeroString" seems to say "000000000" to me...


================
Comment at: llvm/lib/Support/BinaryStreamWriter.cpp:42
+
+  Offset += Str.size();
+  return Error::success();
----------------
zturner wrote:
> 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.
... Oh, ok.

Yeah, that's confusing :)


https://reviews.llvm.org/D30266