[PATCH] Modularize assistant mode

[Sean Silva]

  Do you think that this may benefit in the future from being interactive?


================
Comment at: modularize/ModuleAssistant.cpp:328-330
@@ +327,5 @@
+
+  // Clean up tool output buffer/manager.
+  delete Out;
+  Out = NULL;
+
----------------
Ew. Can you use OwningPtr to capture these lifetime semantics? <http://llvm.org/docs/doxygen/html/classllvm_1_1OwningPtr.html>

================
Comment at: modularize/ModuleAssistant.cpp:265-266
@@ +264,4 @@
+  std::string SafeName = MightBeReservedName;
+  int Index;
+  for (Index = 0; ReservedNames[Index] != NULL; ++Index) {
+    if (MightBeReservedName == ReservedNames[Index]) {
----------------
for (int Index = ...

================
Comment at: modularize/ModuleAssistant.cpp:228-230
@@ +227,5 @@
+  if (Count != 0) {
+    llvm::errs() << "warning: " << FilePath
+                 << " has dependents, meaning module.map won't compile."
+                 << "  Ignoring this header.\n";
+    return true;
----------------
Will the user understand what "dependents" means in this context? I sure don't.

================
Comment at: modularize/ModuleAssistant.cpp:204-207
@@ +203,6 @@
+       I != E; ++I) {
+    llvm::StringRef Path = *I;
+    // Add as a module.
+    if (!addModuleDescription(Path, HeaderPrefix, Dependencies))
+      return false;
+  }
----------------
can fold this to be just `if (!addModuleDescription(*I, ...`

================
Comment at: modularize/ModuleAssistant.cpp:152-158
@@ +151,9 @@
+//
+// 1. Sets up for writing the module map output file using the
+//    "tool_output_file" class from LLVM.
+// 2. Builds an internal tree structure of "Module" objects to represent
+//    the modules, based on the header list input.
+// 3. Writes the module map file by walking the internal module tree by
+//    calling the "output" member of the root module.
+// 4. Finalizes the output.
+bool ModuleAssistantImpl::createModuleMap(
----------------
1-3 here are really helpful comments. 4 is not as helpful; can you improve 4?

================
Comment at: modularize/ModuleAssistant.cpp:95
@@ +94,3 @@
+
+#define INDENT std::string(Indent * 2, ' ')
+
----------------
Ew. Can you use raw_ostream::indent to achieve this more cleanly `OS.indent(...) << ...`? Or maybe a member function `getIndent()`? 

================
Comment at: modularize/ModuleAssistant.cpp:32-38
@@ +31,9 @@
+
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
+#include "llvm/Support/ToolOutputFile.h"
+#include "Modularize.h"
+#include "ModuleAssistant.h"
+#include <vector>
+
----------------
<http://llvm.org/docs/CodingStandards.html#include-style> (the most "local" headers go first).

================
Comment at: modularize/ModuleAssistant.cpp:39-40
@@ +38,4 @@
+#include <vector>
+
+namespace Modularize {
+
----------------
I don't think I've ever a namespace explicitly opened in a .cpp file in the LLVM codebase. Usually there is just a "using namespace Modularize", which works when you then define `Module::foo`.

All classes that are actually declared in the .cpp file (e.g. `class Module` and `class ModuleAssistantImpl` below) are only visible in this .cpp file and should be enclosed in an anonymous namespace <http://llvm.org/docs/CodingStandards.html#anonymous-namespaces>. 

================
Comment at: modularize/ModuleAssistant.cpp:98-99
@@ +97,4 @@
+// Write a module hierarchy to the given output stream.
+bool Module::Output(llvm::raw_fd_ostream &OS, int Indent) {
+  // If this is not the nameless root module, start a module definition.
+  if (Name.size() != 0) {
----------------
Method names are not capitalized <http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly>

================
Comment at: modularize/ModuleAssistant.h:25-36
@@ +24,14 @@
+/// \brief Provides the top-level logic for generating the module map.
+class ModuleAssistant {
+public:
+  // Create and return an object implementing the assistant.
+  static ModuleAssistant *createModuleAssistant();
+  // Create the module map file.
+  virtual bool
+  createModuleMap(llvm::StringRef ModuleMapPath,
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
+                  llvm::StringRef RootModuleName) = 0;
+};
+}
+
----------------
Do you expect to have multiple "module assistant" implementations? Would just a single free function

  bool
  createModuleMap(llvm::StringRef ModuleMapPath,
                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
                  llvm::StringRef RootModuleName)

be sufficient for now?

================
Comment at: modularize/ModuleAssistant.h:29-34
@@ +28,8 @@
+  static ModuleAssistant *createModuleAssistant();
+  // Create the module map file.
+  virtual bool
+  createModuleMap(llvm::StringRef ModuleMapPath,
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
+                  llvm::StringRef RootModuleName) = 0;
+};
----------------
Please add a doxygen comment describing each parameter.

================
Comment at: modularize/ModuleAssistant.h:10-11
@@ +9,4 @@
+///
+/// \file
+/// \brief Module map generation classes and functions.
+///
----------------
This is really unhelpful. At least give the reader a little guidance.

================
Comment at: modularize/ModuleAssistant.h:33
@@ +32,3 @@
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
+                  llvm::StringRef RootModuleName) = 0;
----------------
Where is this `DependencyMap` type coming from? It doesn't seem to be in any of the #includes?

================
Comment at: modularize/ModuleAssistant.h:32
@@ +31,3 @@
+  createModuleMap(llvm::StringRef ModuleMapPath,
+                  llvm::SmallVectorImpl<std::string> &HeaderFileNames,
+                  DependencyMap &Dependencies, llvm::StringRef HeaderPrefix,
----------------
ArrayRef is probably better unless you plan on appending/modifying the SmallVector.

================
Comment at: modularize/ModuleAssistant.cpp:142
@@ +141,3 @@
+  }
+  return NULL;
+}
----------------
LLVM style uses 0 for the null pointer. E.g. <http://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code>. Of course, once we have C++11, nullptr is the obvious thing.


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