[PATCH] D48738: Add the ability to edit stream contents with llvm-pdbutil

[Bob Haarman via Phabricator via llvm-commits]

inglorion added a comment.

This is really useful. Thanks for doing this. I got a couple of questions and comments in-line.



================
Comment at: llvm/include/llvm/DebugInfo/MSF/MSFBuilder.h:125
+  /// existing file untouched.
+  Expected<FileBufferByteStream> commitPartial(StringRef Path,
+                                               MSFLayout &Layout);
----------------
It's not clear to me from the names and the comments what exactly this method does and what the parameters mean. I think something like commitUpdate(StringRef Path, MSFLayout NewLayout) would make it clearer that we're committing an update to an existing MSF file, and the parameter is supposed to be the new layout and not the old one.


================
Comment at: llvm/lib/DebugInfo/MSF/MSFBuilder.cpp:51
+MSFBuilder::MSFBuilder(BumpPtrAllocator &Allocator)
+    : MSFBuilder(4096, msf::getMinimumBlockCount() + 2, true, Allocator) {}
+
----------------
Why +2?


================
Comment at: llvm/tools/llvm-pdbutil/llvm-pdbutil.cpp:688
 
+template <typename OptType> static bool optWasPassed(const OptType &Opt) {
+  return Opt.getNumOccurrences() > 0;
----------------
nit: in llvm/Option/ArgList.h, we have a similar method, but it's called hasArg. I would use the same name here, in part because it's shorter. :-)


================
Comment at: llvm/tools/llvm-pdbutil/llvm-pdbutil.cpp:1250
+  if (optWasPassed(opts::writestream::StreamSize)) {
+    assert(optWasPassed(opts::writestream::StreamFile));
+    cantFail(
----------------
Shouldn't this assert that StreamFile was _not_ passed?


================
Comment at: llvm/tools/llvm-pdbutil/llvm-pdbutil.cpp:1263
+    ExitOnErr(make_error<GenericError>(generic_error_code::invalid_path,
+                                       opts::writestream::StreamFile));
+
----------------
Is there a way to preserve the original error information here, so that one can distinguish between for example a non-existent file and a file one does not have permission to read?


================
Comment at: llvm/tools/llvm-pdbutil/llvm-pdbutil.cpp:1277
+      Writer.writeBytes(arrayRefFromStringRef(StreamContents->getBuffer())));
+  cantFail(OutputBuffer.commit());
+}
----------------
I think this should be ExitOnErr rather than cantFail, similar to other uses of FileBufferByteStream::commit().


https://reviews.llvm.org/D48738