[PATCH] Modularize assistant mode

[Sean Silva]

================
Comment at: modularize/ModuleAssistant.cpp:250
@@ +249,3 @@
+    if (SubModule == 0) {
+      SubModule = new Module(Stem);
+      CurrentModule->SubModules.push_back(SubModule);
----------------
It doesn't look like this has its lifetime managed (same with `RootModule` above). Maybe you could just allocate these into a BumpPtrAllocator?

================
Comment at: modularize/ModuleAssistant.cpp:260-265
@@ +259,8 @@
+
+// Reserved keywords in module.map syntax.
+const char *ModuleAssistantImpl::ReservedNames[] = {
+  "config_macros", "export",   "module", "conflict", "framework",
+  "requires",      "exclude",  "header", "private",  "explicit",
+  "link",          "umbrella", "extern", "use",      0 // Flag end.
+};
+
----------------
Please mention how this list was derived, so that it's clear how to determine if it needs updating (and how to update it).

================
Comment at: modularize/ModuleAssistant.cpp:168-172
@@ +167,7 @@
+//    object.
+bool ModuleAssistantImpl::createModuleMap(
+    llvm::StringRef ModuleMapPath, llvm::ArrayRef<std::string> HeaderFileNames,
+    DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
+    llvm::StringRef RootModuleName) {
+
+  // Set up module map output file.
----------------
Looking at it again, this seems like it should just be a free function with the tool_output_file stack-allocated in it, and passing just the tool_output_file's raw_ostream to the ModuleAssistantImpl constructor. Something like:

    llvm::SmallString<256> FilePath;
    getModuleMapOutputPath(ModuleMapPath, HeaderPrefix, FilePath)
    std::string Error;
    tool_output_file Out(FilePath.c_str(), Error);
    if (!Error.empty()) { ...; return; }
    ...

Is ModuleAssistantImpl necessary? It doesn't seem to be holding any significant state.    

Side note: Notice how there is a "long-distance resource-ownership dependency" between `setUpModuleMapOutput` and `finalizeModuleMapOutput`; usually this indicates that the resource that causes those two routines to be coupled should become a stack-allocated variable, or it should become a by-value member of a class whose lifetime matches the pairing of the two coupled functions.

================
Comment at: modularize/ModuleAssistant.cpp:282-296
@@ +281,17 @@
+                                               llvm::StringRef HeaderPrefix) {
+  // By default, use the path component of the map path.
+  llvm::SmallString<256> HeaderDirectory(ModuleMapPath);
+  llvm::sys::path::remove_filename(HeaderDirectory);
+  llvm::SmallString<256> FilePath;
+
+  // Get the module map file path to be used.
+  if ((HeaderDirectory.size() == 0) && (HeaderPrefix.size() != 0)) {
+    FilePath = HeaderPrefix;
+    // Prepend header file name prefix if it's not absolute.
+    llvm::sys::path::append(FilePath, ModuleMapPath);
+    llvm::sys::path::native(FilePath);
+  } else {
+    FilePath = ModuleMapPath;
+    llvm::sys::path::native(FilePath);
+  }
+
----------------
this should be a static helper function.

================
Comment at: modularize/ModuleAssistant.cpp:332-334
@@ +331,5 @@
+
+  // Delete the tool_output_file object, which causes the
+  // file to be written and then closes the stream.
+  Out.reset();
+
----------------
FYI, the whole point of using an OwningPtr was so that you wouldn't need to manually free it ;)

================
Comment at: modularize/ModuleAssistant.cpp:267-277
@@ +266,13 @@
+
+std::string
+ModuleAssistantImpl::makeNonReservedName(llvm::StringRef MightBeReservedName) {
+  std::string SafeName = MightBeReservedName;
+  for (int Index = 0; ReservedNames[Index] != 0; ++Index) {
+    if (MightBeReservedName == ReservedNames[Index]) {
+      SafeName.insert(0, "_");
+      break;
+    }
+  }
+  return SafeName;
+}
+
----------------
This doesn't depend on any of the class's state, so just make it a static helper function.

Also, since you are intending to modify `MightBeReservedName`, it probably makes more sense to take a SmallVectorImpl&. That would also simplify the interface. The calling sequence would just be `ensureNoCollisionWithReservedName(S);`


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