[PATCH] [PECOFF] Handle .drectve section.

[Reid Kleckner]

  Some peanut gallery comments.


================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:415
@@ +414,3 @@
+        return ec;
+      _directives = std::move(std::string(
+          reinterpret_cast<const char *>(&contents[0])));
----------------
Is it worth doing any kind of check that the contents are string-like and don't contain null bytes?

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:447
@@ +446,3 @@
+  virtual const char *SaveString(const char *str) {
+    auto *res = new (_alloc) std::string(str);
+    return res->c_str();
----------------
Will BumpPtrAllocator call dtors of objects created like this?  What I had in mind as a possible implementation was _alloc.Allocate(strlen(str)+1) plus a memcpy.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:444
@@ -398,1 +443,3 @@
 
+class StringSetSaver : public llvm::cl::StringSaver {
+public:
----------------
This isn't saving strings in a set, it's saving them in a BumpPtrAllocator.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:500
@@ +499,3 @@
+    llvm::raw_string_ostream stream(errorMessage);
+    bool parseFailed = WinLinkDriver::parse(argc, argv, *targetInfo, stream);
+    stream.flush();
----------------
Does this save references to any Arg or ArgList objects?  I'm assuming it doesn't.  An Arg can hold a pointer to an argv string, which would be bad here.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:543
@@ -437,2 +542,3 @@
   ReaderArchive _readerArchive;
+  mutable StringSetSaver _stringSaver;
 };
----------------
This should probably have the same lifetime as argv and thus be a local in handleDirectiveSection.


http://llvm-reviews.chandlerc.com/D1246